Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposal for Native Histogram Text Format #32
Proposal for Native Histogram Text Format #32
Changes from all commits
d74f88a
6ce4170
b1283f1
f80fafa
2920fb2
cec73ef
6ffbe74
7af9827
3717133
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we establish some simple forward compatibility rules for this JSON e.g. Parsers MUST ignore unknown fields? Would help in future.
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.
I would be ok with that, but I am also not sure it is necessary with content negotiation as new fields would require part of a new version of OpenMetrics. I.e. I am unsure if we want to impose the level of strictness a given parser needs to implement, and it might be ok for different parsers to make different choices.
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.
BTW: It's not JSON.
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.
While I like a way of adding stuff that old parsers will just ignore, this concept doesn't exist in OM so far, so it might be weird to introduce it in a sub-set of the protocol. Let's maybe solve that more generally in OM v2.
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.
Cool, thanks. Let's make it clear it's not a JSON somewhere please 🙈
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.
Any suggestions as to how to make it more clear that it is not JSON? In the format definition I have "The value for each series of a native histogram is a custom struct format" and I specifically compare it to JSON with the phrase "Compared to JSON this avoids consistently repeating keys and curly braces" after the type definitions.
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.
Hm, this is interesting. What's "release identifier? I assume you want to version SPEC and native histogram feature separately? Why? Can we make it simpler?
I think I would expect something like:
.. then we could simply do 1.1.1 or 1.2.0 if you have significant histogram change. Let's not create sub versions of every feature, this will make it bit painful to use and maintain. Speaking from experience when we evaled different options for remote write v2: prometheus/docs#2462
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.
See https://semver.org/#spec-item-9, it is just a pre-release, IMO if
version=1.1.0
is there without any sort of prerelease suffix native histograms must be fully supported and there shouldn't be extra feature tags. I would also be ok with something likealpha.0
,alpha.1
, etc... if we don't want to specifically include nativehistogram in the version.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.
I think we need to put some thoughts into versioning. The current proposal does not cover all of native histograms, Exemplars, created timestamps are out-of-scope.
Now, how will content negotiation work if we add these in the future? How will a client library know whether to include a list of exemplars in native histogram text format or not?
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.
I would opt for some way of opting into the feature as an experimental one, without any changes to the standard yet. Based on the experience made, we can make the cut what should land in v1.1 later (including maybe other changes being discussed). In other words: I don't think this design doc has to exactly describe v1.1 already.
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.
I am happy to implement some other versioning scheme, the important part to me is that any exposers add some sort of pre-release identifier until OM 1.1 is actually released, and force ingesters to specify that they are ok with a pre-release version. A features section alongside a pre-release version seems reasonable to me, then becomes unnecessary once 1.1 is released. That allows us some flexibility to make breaking changes to the unreleased features. E.g. if we change around where the list of exemplars are stored, or choose to use a less compact format after feedback.
Content negotiation for features added in 1.2, 1.3, etc... will require client libraries to keep track of the features added in each minor version and leave them out if the requested version is lower. That could mean fall all the way back to 1.0, or maybe have support for a few versions (1.0, 1.1, 1.4) and choose the highest version less than or equal to what is requested by the ingester. E.g. if Prometheus requests 1.3, the client would return 1.1 in that example. 1.0 is the only version that all producers MUST support.
Also note that created timestamps have always been in scope (no change from current text format), and I just added a proposal for exemplar support.