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

Update metrics.py #100

Merged
merged 2 commits into from
Sep 26, 2019
Merged

Update metrics.py #100

merged 2 commits into from
Sep 26, 2019

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Sep 26, 2019

I think this fixes an incorrect import?

I think this fixes an incorrect import?
@vidartf
Copy link
Member Author

vidartf commented Sep 26, 2019

Cleanup after #97, xref #96.

@kevin-bates
Copy link
Member

Hi @vidartf - thanks for looking into this.

I think the actual change relative to this file is to remove it altogether since it was moved from here into prometheus/log_functions.py in this change: jupyter/notebook@03e5dc0 and is no longer present in this location within Notebook.

And then apply this commit jupyter/notebook@7bc7df4 to prometheus/log_functions.py as it appears the cherry-pick for this commit was applied to the "pre-moved" metrics.py instead of prometheus/log_functions.py.

I think this would explain why the tests (relative to Batch-3) are passing and we're not getting failure on the change you proposed - which the correct fix otherwise.

Would you mind updating this PR to delete jupyter_server/metrics.py and apply the import update to juptyer_server/prometheus/log_functions.py?

@vidartf
Copy link
Member Author

vidartf commented Sep 26, 2019

Cleaned up

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.

LGTM - thanks Vidar!

@kevin-bates kevin-bates merged commit 287eded into jupyter-server:master Sep 26, 2019
@vidartf vidartf deleted the patch-1 branch September 26, 2019 15:34
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