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

Prevent re-definition of prometheus metrics #210

Merged
merged 3 commits into from
Apr 25, 2020

Conversation

blink1073
Copy link
Collaborator

Fixes #209

@blink1073 blink1073 closed this Apr 25, 2020
@blink1073 blink1073 reopened this Apr 25, 2020
@blink1073 blink1073 closed this Apr 25, 2020
@blink1073 blink1073 reopened this Apr 25, 2020
@blink1073 blink1073 closed this Apr 25, 2020
@blink1073 blink1073 reopened this Apr 25, 2020
@blink1073
Copy link
Collaborator Author

Terminal cwd failure is unrelated.

@kevin-bates
Copy link
Member

Thanks Steve. I haven't worked with Prometheus and know hardly anything about it (other than what I've just read glossing over some of the docs), but the comments and changes imply that the metrics are scoped at a broader level. Just wondering if we should adjust (namespace) the metric names in juptyer_server so that the notebook-specific metrics are not included should there be a notebook server running on the same node (or network for that matter - since it appears Prometheus performs distributed rollups). I'm assuming we'd want these separated.

As it says on the tin, re-defining these metrics results in an error.

Could you please provide a reference to the tin label? This may help my understanding here - thank you.

If what I said above is how things work (for the most part), but we do want stats cumulated across notebook and js instances, then these changes seem appropriate. My vote would be to keep these separated.

@blink1073
Copy link
Collaborator Author

Hi @kevin-bates, I haven't looked deeply into Prometheus either, I ran into this bug while working an Jupyter Telemetry, since I ended up importing the log.py file twice. I do think it makes sense to scope the parameters down per-server. I don't have the bandwidth to do much more than open this PR at the moment, I'm in the midst of a major refactor of the JupyterLab test suite.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Looks good - addresses the immediate issue. We can revisit scoping via another issue/PR.

@kevin-bates kevin-bates merged commit a2e7a66 into jupyter-server:master Apr 25, 2020
@blink1073 blink1073 added this to the 1.0 Release milestone Sep 17, 2020
Zsailer pushed a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
* Prevent re-definition of prometheus metrics
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.

Prometheus Metrics Duplication Error
2 participants