Repository navigation
Conversation
| var contentType expfmt.Format | ||
| if opts.EnableOpenMetrics { | ||
| contentType = expfmt.NegotiateIncludingOpenMetrics(req.Header) | ||
| contentType = expfmt.NegotiateAccept(req.Header, openMetricsAcceptedFormats...) |
There was a problem hiding this comment.
Open question: Can we introduce OM2 by default (experimental stage)? Given our feature flag on collection side, we probably can and should. cc @dashpole @krajorama
There was a problem hiding this comment.
The worst that can happen is that there's a client_golang version that exposes OM2.0 that's not 100% up to spec. Enabling on server side will mean failed scrape and lost metrics.
I think maybe a mitigation would be to actually enable server side default with a 2.0.1 version, not 2.0.0, so testing can be done with 2.0.0 , but kind of quality gate to 2.0.1 ?
So in short, I think we can enable this now with 2.0.0 , because we have a way to mitigate the above risk.
There was a problem hiding this comment.
- What's 2.0.1? Do we plan that?
- This kind of depends if we plan to make OM 2 a default, highest priority on scrape priority list on Prometheus without major version bump 🤔
There was a problem hiding this comment.
If possible, it would be nice to have it be opt-in until the spec is stabilized. But @krajorama is right that the failure modes aren't that bad. But there will definitely be people that use a newer server and an older (possibly broken) client.
If we do decide that we want to distinguish the rc vs stable versions, I'm not sure 2.0.0 and 2.0.1 make sense, given there could theoretically be breaking changes between them. Ideally it would be something like 2.0.0-rc (or 2.0.0-experimental) and 2.0.0 or something like that.
I don't think we can make OM 2 the default, highest priority on the scrape priority list without a major version bump.
There was a problem hiding this comment.
Yes, using 2.0.0-rc or something would be ideal.
There was a problem hiding this comment.
Interesting, it's true that the protocol is still rc.0 so we could change here to -rc and then update Prometheus side to also put -rc in accept header?
There was a problem hiding this comment.
So, the proposed flow could look like:
- Add
application/openmetrics-text;version=2.0.0-rchere. - Add Accept
application/openmetrics-text;version=2.0.0-rc(as well as the currentapplication/openmetrics-text;version=2.0.0?) on Prometheus behind flag - When 2.0.0 is shipped
3a: Update client_golang to announce both only the non rc by default, deprecate -rc version - On major version bump in Prometheus we make 2.0.0 a default
Alternatives:
A) Aame as above but literal versions e.g 2.0.0-rc.0
B) Exposer ships application/openmetrics-text;version=2.0.0 by default. We risk surprises on major version default swap to 2.0
C) Exposer ships application/openmetrics-text;version=2.0.0 opt-in, then by-default in some next SDK release. This is odd because we defer the same problem (when and how server would change, how much stability we have here etc) to app owners in a programmatic change
There was a problem hiding this comment.
Or one other idea is that the client keeps support for 2.0.0-rc forever, but it is equivalent to 2.0.0 after that is released.
There was a problem hiding this comment.
OK, since this requires prometheus/common etc I allow opt-in for now #2146 - this PR will try to allow default and server opt-in TBD
1ed82b0 to
c864a74
Compare
bwplotka
changed the title
7a9b278 to
c742466
Compare
Support OpenMetrics 2.0 content negotiation when EnableOpenMetrics is true. Add table-driven tests for OpenMetrics 2.0 negotiation and exposition. Signed-off-by: bwplotka <bwplotka@gmail.com>
3 participants
Depends on #2138
Support OpenMetrics 2.0 content negotiation when
EnableOpenMetrics: true.expfmt.FmtOpenMetrics_2_0_0toopenMetricsAcceptedFormatsahead of stable OpenMetrics formats.