Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-47389: app metrics #387

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented Dec 10, 2024

Publish app metrics events.

Phalanx support

@fajpunk fajpunk force-pushed the tickets/DM-47389/app-metrics-for-real branch 2 times, most recently from 52697e2 to 88ac4a8 Compare December 13, 2024 16:40
@fajpunk fajpunk force-pushed the tickets/DM-47389/app-metrics-for-real branch from 88ac4a8 to 8549523 Compare December 13, 2024 19:34
@fajpunk fajpunk requested a review from rra December 13, 2024 19:35
@fajpunk fajpunk marked this pull request as ready for review December 13, 2024 19:36
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

I'm not seeing metrics for the lab spawning process, just for notebook execution. Can we get those as well? I think those are going to be really important. (I don't care about metrics for login and certainly not about the intentional pauses, or for things like creating a web socket.)

@fajpunk
Copy link
Member Author

fajpunk commented Dec 16, 2024

I'm not seeing metrics for the lab spawning process, just for notebook execution. Can we get those as well? I think those are going to be really important. (I don't care about metrics for login and certainly not about the intentional pauses, or for things like creating a web socket.)

Yeah, good call, this is probably one of the most useful events we can have! I added NubladoSpawnLab and NubladoDeleteLab events.

@fajpunk fajpunk force-pushed the tickets/DM-47389/app-metrics-for-real branch from 579a94a to 7b39065 Compare December 16, 2024 18:49
@fajpunk
Copy link
Member Author

fajpunk commented Dec 16, 2024

I'll bring up in co-work whether we want to replace the current timing stuff with sampled Sentry tracing. If we do decide to do that, we may not want some of these as analytical metrics. Notebook cell executions for example, I don't see the value in keeping that data forever, but I do see the value in keeping it short-term to debug why an entire notebook is suddenly slow

@fajpunk fajpunk requested a review from rra December 16, 2024 22:44
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

This looks good, thanks! I'm fairly sure that there are ways to make some of this code simpler, or at least with somewhat sharper separation of concerns, but I think that's an orthogonal refactoring to the work you're doing, and as you say we should probably wait until we've integrated Sentry and then see what the remaining complexity looks like.

@fajpunk fajpunk force-pushed the tickets/DM-47389/app-metrics-for-real branch from bc86c8c to 4601389 Compare December 17, 2024 16:25
@fajpunk fajpunk enabled auto-merge December 17, 2024 16:27
@fajpunk fajpunk merged commit 15ec672 into main Dec 17, 2024
4 checks passed
@fajpunk fajpunk deleted the tickets/DM-47389/app-metrics-for-real branch December 17, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants