feat: implement gauge and counter support for OpenMetrics 2.0#894
feat: implement gauge and counter support for OpenMetrics 2.0#894dashpole wants to merge 3 commits into
Conversation
Signed-off-by: David Ashpole <dashpole@google.com>
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
For common, given v0 I would just comment on the method. If we want to be fancy we can generate comment in the exposition format too (:
Looks good so far, good start (see comments) - only question is do we iterate on this on main or some feature branch - no strong opinion, assuming commentary is clear.
Thanks
Signed-off-by: David Ashpole <dashpole@google.com>
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
looks good to me so far. My general opinion is that if some scraper asks for openmetrics 2.0, they should get it... Hypothetically I could see requiring the version number "2.0.0rc" or "1.9.9" just in case?
|
Lots of discussion at the OM wg. We will add a NegotiateIncluding() function that takes a list of supported protocols so we don't end up with NegotiateIncludingOpenMetricsAndOpenMetrics2 ... 😆 . Then the client can decide when it wants to add support, and how it wants to gate it (behind |
|
I added a NegotiateIncluding function. I didn't deprecate the old functions (e.g. NegotiateIncludingOpenMetrics), but we can do that in a follow-up if we want to. |
Signed-off-by: David Ashpole <dashpole@google.com>
|
I think that's great, the experimental factor at this level should be a commentary. As per discussion on Slack I think we decided to follow:
|
| // temporary and will disappear once FmtOpenMetrics is fully supported and as | ||
| // such may be negotiated by the normal Negotiate function. | ||
| func NegotiateIncludingOpenMetrics(h http.Header) Format { | ||
| return NegotiateIncluding(h, FmtOpenMetrics_1_0_0, FmtOpenMetrics_0_0_1) |
| }, | ||
| Counter: &dto.Counter{ | ||
| Value: proto.Float64(1027), | ||
| CreatedTimestamp: ×tamppb.Timestamp{Seconds: 1234567890}, |
There was a problem hiding this comment.
Let's add a test for counters where ST has ms resolution, e.g.:
CreatedTimestamp: ×tamppb.Timestamp{Seconds: 1234567890, Nanos: 987654321},
To test that the result has the precision:
http_requests_total{method="GET",code="200"} 1027 st@1234567890.9876542
Maybe we can discuss if we only want prec==3 for this case. But keep that discussion separate from the PR.
| } | ||
|
|
||
| if exemplar != nil && len(exemplar.Label) > 0 { | ||
| n, err = writeExemplar(w, exemplar) |
There was a problem hiding this comment.
I don't think we can call this function as is. It treats the exemplar timestamp as optional. I think we need to drop exemplars without timestamp for om2.0. https://prometheus.io/docs/specs/om/open_metrics_spec_2_0/#exemplars
There was a problem hiding this comment.
According to LLM, client_golang at least always sets the timestamp, so it will not be breaking to do this, i.e. drop exemplars without timestamp.
| { | ||
| Counter: &dto.Counter{ | ||
| Value: proto.Float64(1027), | ||
| Exemplar: &dto.Exemplar{ |
There was a problem hiding this comment.
Invalid test, the exemplar must have a timestamp.
| }, | ||
| }, | ||
| out: `# TYPE http_requests_total counter | ||
| http_requests_total 1027 # {trace_id="1234"} 1.0 |
There was a problem hiding this comment.
Let's also add start time to verify the order.
And possibly the sample timestamp as well, although I don't know how to do that.
4 participants
Part 1 of #893
Histograms and summaries are left for follow-ups.
Open Questions:
@ywwg @bwplotka @krajorama