Skip to content

Add telemetry events for Jupyterhub REST APIs - #3218

Draft
kiendang wants to merge 20 commits into
jupyterhub:mainfrom
kiendang:event-schemas
Draft

Add telemetry events for Jupyterhub REST APIs#3218
kiendang wants to merge 20 commits into
jupyterhub:mainfrom
kiendang:event-schemas

Conversation

@kiendang

Copy link
Copy Markdown

Add event schemas + code for emitting those events for the endpoints in Jupyterhub REST APIs.

@kiendang
kiendang marked this pull request as ready for review November 12, 2020 08:58
Comment thread jupyterhub/apihandlers/auth.py Outdated
self.eventlog.record_event(
eventlogging_schema_fqn('auth-token-action'),
1,
{'action': 'get', 'token_id': token},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't include the token itself in events. This should be orm_token.api_id

Comment thread jupyterhub/apihandlers/auth.py Outdated
self.eventlog.record_event(
eventlogging_schema_fqn('auth-token-action'),
1,
{'action': 'create', 'token_id': api_token,},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about token id

Comment thread jupyterhub/apihandlers/users.py Outdated
1,
{
'action': 'create',
'target_user': {'username': user.name, 'admin': False},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'target_user': {'username': user.name, 'admin': False},
'target_user': {'username': user.name, 'admin': admin},

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

Comment thread jupyterhub/apihandlers/users.py Outdated
1,
{
'action': 'get',
'target_user': {'username': user.name, 'admin': user.admin},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to use the same fields as the user model, so 'name' here so it's user.name instead of the redundant user.username

'requester': self.current_user.name,
'target_user': {'username': user.name, 'admin': user.admin},
'prior_state': prior_state,
'auth_state_change': 'auth_state' in data,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread jupyterhub/apihandlers/users.py Outdated
1,
{
'action': 'get',
'target_user': user.name,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should target_user always have the same schema ({name, admin})?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do the same for requester everywhere as well?

Comment thread jupyterhub/apihandlers/users.py Outdated
'action': 'delete',
'target_user': user.name,
'requester': self.current_user.name,
'token_id': token_id,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'token_id': token_id,
'token_id': token.api_id,

@kiendang

Copy link
Copy Markdown
Author

@Zsailer

@manics

manics commented May 21, 2022

Copy link
Copy Markdown
Member

I've marked this as draft for now.

@minrk @yuvipanda What do you think are the next steps for this PR (and presumably jupyter-server/jupyter_server#364)?

@minrk

minrk commented May 25, 2022

Copy link
Copy Markdown
Member

I think it just needs review (and now conflict resolution). @yuvipanda do you want to have a look over the schemas? I can handle the conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants