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

Add prometheus metric with version information #1467

Merged
merged 18 commits into from
Nov 4, 2024

Conversation

yuvipanda
Copy link
Contributor

@yuvipanda yuvipanda commented Oct 29, 2024

  • Allows for collection of jupyter server version as a prometheus metric, and filtering of various other metrics based on server version.
  • Builds on top of conventions in other servers that expose version (or build) info. For example, kubernetes puts out this metric:

    kubernetes_build_info{build_date="2024-09-09T04:15:23Z", compiler="gc",
    git_commit="fa081458abcdbbc3ddfc3f99546ae5ea45aece17", git_tree_state="clean",
    git_version="v1.29.8-gke.1700", go_version="go1.22.5 X:boringcrypto",
    instance="10.128.0.50:443", job="kubernetes-apiservers", major="1", minor="29",
    platform="linux/amd64"}
    Most of these parameters are not particularly relevant to us, but version is. So this PR only adds version - future labels can be added as needed.

  • Fix circular import of jupyter_server.prometheus.metrics when nbclassic shims notebook.prometheus to be the same as jupyter_server.prometheus

yuvipanda and others added 4 commits October 28, 2024 19:54
Allows for collection of jupyter server version as a prometheus
metric, and filtering of various other metrics based on server
version.

Builds on top of conventions in other servers that expose
version (or build) info. For example, kubernetes puts out this
metric:

> kubernetes_build_info{build_date="2024-09-09T04:15:23Z", compiler="gc",
> git_commit="fa081458abcdbbc3ddfc3f99546ae5ea45aece17", git_tree_state="clean",
> git_version="v1.29.8-gke.1700", go_version="go1.22.5 X:boringcrypto",
> instance="10.128.0.50:443", job="kubernetes-apiservers", major="1", minor="29",
> platform="linux/amd64"}

Most of these parameters are not particularly relevant to us, but
version is. So this PR only adds version - future labels can be
added as needed.
@yuvipanda
Copy link
Contributor Author

Not really sure why it's failing in nbclassic now, although I see nbclassic is doing some fun import schenanigens...

@yuvipanda
Copy link
Contributor Author

This was a nice debug :)

Due to https://github.com/jupyter/nbclassic/blob/9ae4cb137ea46c9a29cdcf56032dae624f6ff8d8/nbclassic/shim_notebook.py#L72, notebook.prometheus.metrics is actually the same as jupyter_server.prometheus.metrics! So the existing code is actually a circular import:

Traceback (most recent call last):
  File "/Users/yuvipanda/.local/share/virtualenvs/jupyter_server/bin/jupyter-nbclassic", line 5, in <module>
    from nbclassic.notebookapp import main
  File "/Users/yuvipanda/.local/share/virtualenvs/jupyter_server/lib/python3.10/site-packages/nbclassic/__init__.py", line 36, in <module>
    shim_notebook()
  File "/Users/yuvipanda/.local/share/virtualenvs/jupyter_server/lib/python3.10/site-packages/nbclassic/shim_notebook.py", line 80, in shim_notebook
    from jupyter_server import log
  File "/Users/yuvipanda/code/jupyter_server/jupyter_server/log.py", line 15, in <module>
    from .prometheus.log_functions import prometheus_log_method
  File "/Users/yuvipanda/code/jupyter_server/jupyter_server/prometheus/log_functions.py", line 3, in <module>
    from .metrics import HTTP_REQUEST_DURATION_SECONDS  # type:ignore[unused-ignore]
  File "/Users/yuvipanda/code/jupyter_server/jupyter_server/prometheus/metrics.py", line 8, in <module>
    from notebook.prometheus.metrics import (
  File "/Users/yuvipanda/code/jupyter_server/jupyter_server/prometheus/metrics.py", line 8, in <module>
    from notebook.prometheus.metrics import (
ImportError: cannot import name 'HTTP_REQUEST_DURATION_SECONDS' from partially initialized module 'notebook.prometheus.metrics' (most likely due to a circular import) (/Users/yuvipanda/code/jupyter_server/jupyter_server/prometheus/metrics.py)

The ImportError catch was masking this!

@yuvipanda
Copy link
Contributor Author

I'm not actually sure how to proceed here though, because I'm not fully sure I understand why https://github.com/jupyter/nbclassic/blob/9ae4cb137ea46c9a29cdcf56032dae624f6ff8d8/nbclassic/shim_notebook.py is doing what it's doing. I think it's patching "notebook." things to point to things in jupyter_server, but then it means we can't actually import notebook things in jupyter_server anymore, right? Otherwise you'll end up with circular imports exactly like we are now...

@yuvipanda
Copy link
Contributor Author

Oooooh, funkier! So notebook.prometheus no longer exists in notebook v7, so we should update the check here.

@yuvipanda
Copy link
Contributor Author

Quite funny, as we can't actually rely on version info to see if we're on notebook v6, because nbclassic shims version too it looks like :D

This commit caused the *notebook* downstream version check
to fail, which makes sense?

This reverts commit 40029a7.
@yuvipanda
Copy link
Contributor Author

I don't see where nbclassic shims notebook._version, but if I print out notebook._version.version_info in the test that was failing, I get (2, 15, 0, '.dev0') so that shimming must be happening somewhere (see https://github.com/jupyter-server/jupyter_server/actions/runs/11567104531/job/32196871585)

@yuvipanda
Copy link
Contributor Author

All tests pass! That was rewarding.

@yuvipanda
Copy link
Contributor Author

codespell crampin my styl

@Zsailer Zsailer self-requested a review November 1, 2024 04:13
@Zsailer Zsailer self-assigned this Nov 1, 2024
@Zsailer
Copy link
Member

Zsailer commented Nov 1, 2024

I love @yuvipanda PRs :)

I'm really swamped this week, but will do my best to give this a solid review tomorrow.

Copy link
Member

@Zsailer Zsailer 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 @yuvipanda! Thank you!

@Zsailer Zsailer merged commit 32679b9 into jupyter-server:main Nov 4, 2024
36 of 37 checks passed
yuvipanda added a commit to yuvipanda/jupyter_server that referenced this pull request Nov 4, 2024
yuvipanda added a commit to yuvipanda/jupyter_server that referenced this pull request Nov 4, 2024
@yuvipanda
Copy link
Contributor Author

ty @Zsailer! This encourages me to add more useful version info metrics, like #1470

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

Successfully merging this pull request may close these issues.

2 participants