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
Prometheus metrics #2610
Prometheus metrics #2610
Changes from 2 commits
d6c5027
7576247
ee723d7
92f195f
53c34d4
8afbcae
4ce3f9c
2b5a972
89eef1f
0312a1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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 think this is a good metric to track independently of new/close
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.
Is this something we should have a follow-up issue for?
What would be your suggestion for an approach to remedy this confusion?
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.
it would need a follow up; we can try to make a metrics registry per listener in order to avoid this
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.
We are moving initialization code here from
InitMetrics
toinit
. I agree with the conflict about having global metric and an exposed initializer, but should we leave any refactor related to this to its own PR and keep this one only for the Prometheus changes? Is there any need to change fromInitMetrics
toinit
on this PR?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 didn't move the initilization code between functions, i just moved the init function closer to the top of file
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.
Should we consider using a prometheus counter vector with labels to track error types?
I know that for prometheus's query language it would make getting total error counts simpler, but I don't know if that translates to our planned connection or not
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.
Good point, it would make sense, yes, but we should still maintain the old metrics for backwards compatibility. If adding labels would complicate it, I would continue without labels for the error types.
Maybe we can have some methods like
newTypedCounter(registry, "error", "limit_rate")
, that for the old metrics they just keep the old names (limit_rate
), but for the prometheus metrics they use the first parameter as error and the second astype
label (error{type="limit_rate_count"}
).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.
yeah I think it's a bit too complex at the moment to do so, i might try to add the prometheus metrics as a middleware layer instead
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.
Well explained - appreciate the comment 🙏
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.
Nice addition over the POC 👍