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

Soft-deprecate /metrics endpoint #68

Merged
merged 3 commits into from
Nov 19, 2020
Merged

Soft-deprecate /metrics endpoint #68

merged 3 commits into from
Nov 19, 2020

Conversation

yuvipanda
Copy link
Collaborator

/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

/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
yuvipanda added a commit to yuvipanda/datahub-old-fork that referenced this pull request Nov 17, 2020
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
@jtpio
Copy link
Member

jtpio commented Nov 17, 2020

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 /metrics: #58

@@ -11,7 +11,7 @@

setuptools.setup(
name="nbresuse",
version="0.3.6",
version="0.4.0",
Copy link
Member

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?

Copy link
Collaborator Author

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?

Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Member

@jtpio jtpio Nov 19, 2020

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

yuvipanda added a commit to yuvipanda/datahub-old-fork that referenced this pull request Nov 18, 2020
- /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.
Copy link
Member

@jtpio jtpio left a 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!

@yuvipanda
Copy link
Collaborator Author

Thanks for reviewing & engaging, @jtpio! Would you feel comfortable merging this PR? :D

@jtpio
Copy link
Member

jtpio commented Nov 19, 2020

Sure. Was holding a bit more in case you had some input of the summary in #68 (comment)

@jtpio jtpio merged commit 1d7b8d4 into master Nov 19, 2020
@jtpio
Copy link
Member

jtpio commented Nov 19, 2020

Would you like to proceed with the release? Otherwise I can do it later today / tomorrow.

@jtpio jtpio deleted the legacy-api-endpoint branch November 19, 2020 16:45
@yuvipanda
Copy link
Collaborator Author

@jtpio nope, would be great if you could do the release!

@jtpio jtpio added this to the 0.4.x milestone Nov 20, 2020
@jtpio jtpio mentioned this pull request Nov 23, 2020
13 tasks
@manics manics mentioned this pull request Nov 26, 2020
8 tasks
shaneknapp pushed a commit to berkeley-dsep-infra/biology-user-image that referenced this pull request Sep 6, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
shaneknapp pushed a commit to berkeley-dsep-infra/eecs-user-image that referenced this pull request Sep 11, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
shaneknapp pushed a commit to berkeley-dsep-infra/julia-user-image that referenced this pull request Sep 12, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
shaneknapp pushed a commit to berkeley-dsep-infra/data102-user-image that referenced this pull request Sep 19, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
shaneknapp pushed a commit to berkeley-dsep-infra/data100-user-image that referenced this pull request Sep 25, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
shaneknapp pushed a commit to berkeley-dsep-infra/datahub-user-image that referenced this pull request Sep 25, 2024
Brings in jupyter-server/jupyter-resource-usage#68 so
we can move towards getting prometheus metrics from notebooks.
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