Add global-ratelimiter aggregator-side metrics #6171
Conversation
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -1729,7 +1731,8 @@ var ScopeDefs = map[ServiceIdx]map[int]scopeDefinition{ | |||
| HashringScope: {operation: "Hashring"}, | |||
|
|
|||
| // currently used by both frontend and history, but may grow to other limiting-host-services. | |||
| FrontendGlobalRatelimiter: {operation: "GlobalRatelimiter"}, | |||
| GlobalRatelimiter: {operation: "GlobalRatelimiter"}, | |||
There was a problem hiding this comment.
I suggest using lower-case. m3 lowercase everything, and usage of CamelCase makes it harder to find metrics.
There was a problem hiding this comment.
I mostly agree, lower-case is nicer for lots of things, but all of our "operation" tags are title-case at the moment. Not sure there's much benefit to breaking that long-held pattern.
is it just for code-grepping difficulty, or does it make metric-querying harder somehow?
There was a problem hiding this comment.
(this is pretty easy to change if we do want to change things later, nothing we have now depends on this for monitoring/log search/etc)
| @@ -538,3 +578,18 @@ func weighted[T numeric](newer, older T, weight float64) T { | |||
| type numeric interface { | |||
| constraints.Integer | constraints.Float | |||
| } | |||
|
|
|||
| // non-sync version of sync.Once, for easier unlocking | |||
There was a problem hiding this comment.
I'd make it explicit in the comments there "Do NOT use this for concurrent cases"
There was a problem hiding this comment.
that is part of why I didn't make it a public type, yea. not intended for reuse.
It would've been useful to have some of these while checking the initial rollout and troubleshooting, and the rest seem possibly-also-useful and easy to collect. I've also adjusted the histogram buckets shared by other global ratelimiter stuff - nothing is planned that will be actually sensitive to the values, so this should be harmless. It's more for general "wait why do we have 100x more/fewer than we expected"-style discoveries.
It would've been useful to have some of these while checking the initial rollout and troubleshooting, and the rest seem possibly-also-useful and easy to collect.
I've also adjusted the histogram buckets shared by other global ratelimiter stuff - nothing is planned that will be actually sensitive to the values, so this should be harmless. It's more for general "wait why do we have 100x more/fewer than we expected"-style discoveries.