-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
metrics: Enforce smaller set of method labels #4545
metrics: Enforce smaller set of method labels #4545
Conversation
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.
Awesome, thanks Dave! Just a couple things
@mholt I've merged the common sanitize functions into an |
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.
Thanks Dave! LGTM except one question I had. I might just not be understanding it fully but I am curious your thoughts. Really appreciate it!
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.
Still don't love the method map with repeated methods, but has my approval for merge either way. Thanks!
Signed-off-by: Dave Henderson <[email protected]>
Signed-off-by: Dave Henderson <[email protected]>
Signed-off-by: Dave Henderson <[email protected]>
dcafbf4
to
7ca5921
Compare
Nice work Dave, thank you so much for helping with this! |
Protects against unbounded cardinality on the method label.
Note: I'm finding myself copying code again between the two metrics endpoints - how do we feel about
internal/
? I ask because there's no use of it yet... Otherwise I think I'd rather throw the common code in aninternal/metrics
package or something like that...Signed-off-by: Dave Henderson [email protected]