prometheus
OM text exposition for NH
For the sake of separation of concerns, this PR focuses only on the OM exposition format for native histograms and it does so by mocking native histograms in tests i.e. it uses the helper function custom_collector (as it is already done for gauge histograms) instead of the observe function. I will carry out the implementation of the "complete" support for native histograms in client_python in another iteration (I'm already on it).
Incidentally, in the exposition logic, the #HELP line gets appended before the #TYPE line, whereas in the parser logic is the other way around; also, in the exposition logic the value is always a float in the OM format, whereas in the parser it doesn't seem to be the norm. In this PR I follow the behaviour already existing in the exposition logic. Let me know if I should change anything in this regard.
P.S. Of course this way of arranging the tests is just temporary; in the next iteration, they will be modified to use observe accordingly.
Edit: this PR addresses part of the issue https://github.com/prometheus/OpenMetrics/issues/279
@csmarchbanks and @krajorama , thanks a lot for your reviews! Before I proceed addressing your change requests, let me know how you think class Sample should look like, as per my comment above. Many thanks in advance!
FYI, I've just resumed working on addressing your requests for changes today. Thanks for bearing with me :)
I'll solve the linting problems over the weekend or so. I tried to address most of your requests for change. The biggest change I had to make was adding an attribute nh_exemplars to the class NativeHistogram which is just a sequence of the already existing Exemplar class, so "option 1" but without creating an ad hoc NativeHistogramExemplar class. I added just one test for that, but I'll be happy to add more if you don't have any objections to the approach I took.
EDIT: The nh_exemplars are assigned to the exemplarstr.
I have started the OpenMetrics 2.0 specification. Might be worth aligning on that first: prometheus/docs#2634 .
Some things that came up due to this work are listed in a doc pointed out by the PR: https://docs.google.com/document/d/1FCD-38Xz1-9b3ExgHOeDTQUKUatzgj5KbCND9t-abZY/edit?tab=t.nvipu0hrna3u
Thank you for the heads-up! I'll read the PR + doc and align the code with the new decisions as soon as possible.
@krajorama I started by changing the printing order of the spans and deltas. I could see that the naming for spans and deltas (along with other topics) is still under the "open questions" section though. Shall I just go for n_spans, n_deltas etc regardless?
Following yesterday's OM 2.0 working group meeting, I guess I can continue implementing changes gradually in this PR every time consensus gets reached on the open questions regarding native histograms (no matter how long it will take for the group to decide). At the same time I could also work to get the parser "up to speed" in a similar way (I think the case when there are nh with exemplars to be parsed isn't yet contemplated, for example), in a separate, new PR. Do you have any objections or other plans @csmarchbanks @krajorama ? Thanks in advance!
P.S. What I mean is that I'm fine with this or any similar PR of mine to stay open for as long as it takes :)
Thank you for sharing!
My personal preference would be to merge this before too long, mark everything as experimental (which it already is I believe), and then iterate on it before we remove the experimental marker. That avoids quite so large of pull requests and constant conflicts.
Thank you for sharing!
My personal preference would be to merge this before too long, mark everything as experimental (which it already is I believe), and then iterate on it before we remove the experimental marker. That avoids quite so large of pull requests and constant conflicts.
Oh right! I forgot about the experimental option. Let me solve these conflicts, then I guess we can merge, if everything looks good.
Following up on the Slack conversation in the prometheus-histogram-dev channel, @krajo did you want for me to change the timestamp implementation and to emit native histograms only when OM 2.0 is requested, in this PR or in another iteration?