-
Notifications
You must be signed in to change notification settings - Fork 17
feat: support info metrics #267
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
base: release-2.53.5-gmp
Are you sure you want to change the base?
Conversation
bwplotka
left a 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.
That's generally should be it, but also test will tell us more.
I'd recommend adding one to https://github.com/GoogleCloudPlatform/prometheus/blob/release-2.53.5-gmp/google/internal/promqle2etest/gcm_test.go
Since SDK does not support info metrics you might need some slight change in how we setup test framework to inject custom text format 🤔 that might be the main work here. If I were you I would attempt to extend (you are welcome to contribute too!) https://github.com/prometheus/compliance/blob/7667b56d892fbb4f4252287e50bbad6d2a8f6893/promqle2e/promqle2e.go#L435 so it accepts []string with OpenMetrics raw TEXT to show (and change) on every scrape and expect some result.
This would benefit the promqle2e and our environment, but if you find a different way to test this (e.g. extending our side local export GCM helper:
| func (l *runningLocalExportWithGCM) IngestSamples(ctx context.Context, t testing.TB, recorded [][]*dto.MetricFamily) { |
|
|
||
| case model.MetricTypeInfo: | ||
| protos.gauge = newSeries( | ||
| c.getMetricType(metricName, gcmMetricSuffixGauge, gcmMetricSuffixNone), |
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.
One question to answer before we move forward: Should we reuse gauge suffixgcmMetricSuffixGauge or add some /info suffix and if that decision even matter now (e.g. we can easily change later). Gauge would totally work for now. The only reason to do info is to be forward compatible with potential special handling of info in PromQL later on. Similar for stateset.
FYI: the definitions of those rare types are here: https://prometheus.io/docs/specs/om/open_metrics_spec/#info
I think it's fine to have gauge for both now, because:
- There are no clear plans how to have special PromQL handling for info and stateset. There are small discussions on some storage efficiency gains for native info/stateset samples, but that's not impacting us.
- If, ever, we will make breaking changes to Prometheus to suddenly treat info and stateset specially on PromQL it will be a big thing and in 4.0 Prometheus, soo.. YAGNI.
- Otel collector is already assuming a "sum" (gauge) AFAIK
@lyanco @realschwa @dashpole to double check my thinking (:
FYI: There's some discussions to add info and stateset natively to SDK (https://cloud-native.slack.com/archives/CC6CPDEJV/p1762847796024439), so they will be more popular, but again, will be treated as a gauge on consumption.
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.
+1 to YAGNI - gauge seems fine for now and it matches other info-like metrics that already exist (e.g. the ones from ksm).
Do these metrics typically end in _info? If so we can always special case metrics that end in _info/gauge if we ever need to. Or later on if /info becomes necessary we could always change how we write these metrics to include a /info suffix, or double-write (cardinality is not high for these, so cost isn't either), or something.
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.
The info suffix is specified for OpenMetrics. Not sure how well that translates to real-world usage.
Add support for INFO metrics by converting them to Gauges.