-
Notifications
You must be signed in to change notification settings - Fork 103
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
Soft-deprecate /metrics endpoint #68
Conversation
/metrics is where notebook server publishes prometheus metrics. Currently, if nbresuse is installed, it's impossible to get prometheus metrics out of running notebooks. Just removing /metrics caused issues with third-party code (#36) that depended on the JSON API from the endpoint, and so was reverted. Instead, we 'soft' deprecate that endpoint. 1. Re-add /api/metrics/v1 endpoint, with exact same semantics as /metrics. This is enabled unconditionally. 2. /metrics is also enabled by default, but can be turned off with a notebook config. This allows admins who need prometheus metrics to turn it on. 3. The bundled notebook extension will read from /api/metrics/v1 endpoint, so for those users things will 'just work'. The path to removing /metrics is: 1. Release this as 0.4.0 even though there aren't any breaking changes. This makes it easier for code using legacy endpoints to simply pin to nbresuse<0.4.0 2. Find third party users relying on /metrics, and simply change them to use /api/metrics/v1 - should be a fairly simple change. They can then be pin to nbresuse>=0.4.0 3. When we're sure enough packages have switched, we can release a 0.5.0 where /metrics is disabled by default, but enablable with a config. 4. Eventually, we can simply remove /metrics and the associated config. Ref #36
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Thanks! The path to removing /metrics looks good and similar to what was summarized in the roadmap, with one more step of deprecation and backward compatibility with |
@@ -11,7 +11,7 @@ | |||
|
|||
setuptools.setup( | |||
name="nbresuse", | |||
version="0.3.6", | |||
version="0.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of a separate release commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a separate commit, just in the same PR. Should probably be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking that we might want to add a couple more changes between this PR and the release.
For example moving the lab extension here to the nbresuse repo: jupyterlab/jupyterlab#9363
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtpio hmm, ideally I'd like to do a quick release here, and then maybe do a 0.5 with other changes - so only change in 0.4 is the deprecation. Since existing JupyterLab installations will work with 0.4 anyway, it should be probably ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
So to summarize:
- 0.4.0 with the soft-deprecated endpoint. Users in lab 2.x will still see the indicator because
/metrics
is still available - if the status bar item is removed from core for lab 3.0, then we release nbresuse 0.5.0 with this change: Add a JupyterLab extension for the memory usage status bar item #69. With 0.5.0 removing the
/metrics
endpoint (or enablable with a config) so users with the lab 2.x + nbresuse combination don't see the indicator twice in the status bar
- /metrics now responds with prometheus metrics, rather than nbresuse metrics (jupyter-server/jupyter-resource-usage#68) - Notebook Server's /metrics endpoint is now available without authentication, so prometheus can scrape those metrics (jupyter/notebook#5870) - Annotations are set up on the user pods to tell prometheus the port and path to scrape. - NetworkPolicy is set up to allow prometheus server to hit the singleuser server and the hub pods. Note that this required manually editing netpol in prometheus - should add that feature to the prometheus chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot Yuvi for this!
Thanks for reviewing & engaging, @jtpio! Would you feel comfortable merging this PR? :D |
Sure. Was holding a bit more in case you had some input of the summary in #68 (comment) |
Would you like to proceed with the release? Otherwise I can do it later today / tomorrow. |
@jtpio nope, would be great if you could do the release! |
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
Brings in jupyter-server/jupyter-resource-usage#68 so we can move towards getting prometheus metrics from notebooks.
/metrics is where notebook server publishes prometheus metrics.
Currently, if nbresuse is installed, it's impossible to get
prometheus metrics out of running notebooks.
Just removing /metrics caused issues with third-party
code (#36) that depended
on the JSON API from the endpoint, and so was reverted.
Instead, we 'soft' deprecate that endpoint.
/metrics. This is enabled unconditionally.
a notebook config. This allows admins who need prometheus metrics
to turn it on.
endpoint, so for those users things will 'just work'.
The path to removing /metrics is:
This makes it easier for code using legacy endpoints to simply pin
to nbresuse<0.4.0
to use /api/metrics/v1 - should be a fairly simple change. They can
then be pin to nbresuse>=0.4.0
where /metrics is disabled by default, but enablable with a config.
Ref #36