Include metrics for configured limit overrides#1089
Include metrics for configured limit overrides#1089joe-elliott merged 8 commits intografana:mainfrom
Conversation
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
3 times, most recently
from
e88e046 to
9cf9b2a
Compare
zalegrala
requested review from
KMiller-Grafana,
annanay25,
dgzlopes,
joe-elliott,
mapno,
mdisibio and
yvrhdn
as code owners
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
from
dd82ad8 to
f210535
Compare
|
I've added a metric for default limits as well. |
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
from
91b548a to
5c1d86f
Compare
|
I've made an update to implement the Overrides module as a Collector. We could adjust to have the exporter be its own module /package like is done in the cortex code, but I think that might be a little more than we need for this. What do you think? |
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
2 times, most recently
from
58e0c72 to
5a33173
Compare
| var manager *runtimeconfig.Manager | ||
| subservices := []services.Service(nil) | ||
|
|
||
| metricDefaultsLimits.WithLabelValues("max_local_traces_per_user").Set(float64(defaults.MaxLocalTracesPerUser)) |
There was a problem hiding this comment.
make max_local_traces_per_user, etc. constants
There was a problem hiding this comment.
This is addressed in the last commit.
There was a problem hiding this comment.
I'm still seeing individual static strings.
There was a problem hiding this comment.
Oh sorry, I thought you meant prometheus.MustNewConstMetric to make them constants. You mean make the metric name string a constant to be referenced by both the defaults and the overrides, right?
There was a problem hiding this comment.
yup. I just meant to make a constant string that they both use. I have no idea what MustNewConstMetric is but let's just follow whatever Cortex does here.
There was a problem hiding this comment.
Yep that's what they are doing. This promehteus.Collector pattern is new to me, but the const bit is document here: https://prometheus.io/docs/instrumenting/writing_exporters/#collectors
There was a problem hiding this comment.
In Cortex the exporter is also a module, but as it is now we aren't doing that. If we want to do that, I think we just need to make a limits/exporter package and glue it up, but not sure how far to go with modules. It would be our first "exporter" module, but it could be a pattern if we want to structure the exporters as modules. Seems to me that they belong with the module they support, rather than a module in themselves. What do you think?
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
from
5faecd7 to
822c19c
Compare
zalegrala
force-pushed
the
overridesLimitsMetrics
branch
from
822c19c to
ef029c2
Compare
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
nice! i think we're there!
3 participants
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]