Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[wip] prometheus monitoring support #26837
[wip] prometheus monitoring support #26837
Changes from 2 commits
90bdf7d
7ad93fa
0694926
c1f8270
5034156
172ef37
2be4e6a
eb8afe3
54e91c0
54f2d1b
eba69c4
01fbc07
450f211
2c3edef
8664161
0bcedea
9ba3d4a
904e358
d68e104
5c00052
e00b7e1
20e8a61
0ab0006
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this doesn't look like it will always return
bool
(it will probably return a string orNone
). If you want a bool you could doBut that doesn't seem very Pythonic. I'd probably drop the type hint.
However, I'm not very familiar with the conventions around type hints. Does
-> bool
imply that returned object can be treated like a bool (most objects support that), or that the returned value is alwaysTrue
orFalse
? This is one reason I'm not a fan of type hints in Python.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.
Can this be simplified?
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.
True, this was copied from the Prometheus client.
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.
This is required by Prometheus.
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.
F401 'corehq.util.metrics.metrics.MetricBase' imported but unused
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.
Oof, multiple inheritance with diamonds... I don't love this.
I'd rather have simple functional call patterns like our existing
datadog_counter(..., tags=...)
(made generic so as not to have "datadog" in the name) rather than changing all of our metrics calls to use a new metrics system with extensive use of inheritance.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.
Yes, the inheritance isn't great here. The alternative is a bit more duplicate code but not a lot. I'll see what I can do to make it better if we stay on this course.
If we want to keep it functional then we would need to have a centralized metrics singleton that would always return the same metric class when called (in the case of Pometheus). It would also mean that every call to the metric would need to pass in all required args (which is the case now anyway):
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.
I've resolved this by removing the need to create new copies of the class when applying tags: