Skip to content
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

Clarify compatibility and content negotiation #273

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

krajorama
Copy link
Member

Following the dev summit on 13th September in Berlin.

https://docs.google.com/document/d/1uurQCi5iVufhYHGlBZ8mJMK_freDFKPG0iYBQqJ9fvA/edit#bookmark=id.kj599un8yoj7

From the dev summit document:

CONSENSUS: We intend to add native histograms as a part of OpenMetrics 1.1. A major version bump is not required.
CONSENSUS: We agree that the understanding of backwards compatibility is that a A.y parser can also parse an A.x format. But the A.x parser doesn’t need to parse the A.y format. Where y > x, A, x, y being integers
CONSENSUS: OpenMetrics doesn’t need a major version bump if the changes are backwards compatible.

Notes:

  • this is my understanding
  • I'm suggesting a more comprehensive update than agreed in the dev summit
  • I think the document should actually state its own version (1.0.0) , but I'll leave that to a different PR

Following the dev summit on 13th September in Berlin.

https://docs.google.com/document/d/1uurQCi5iVufhYHGlBZ8mJMK_freDFKPG0iYBQqJ9fvA/edit#bookmark=id.kj599un8yoj7

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Copy link
Member

@RichiH RichiH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I did a first run; I am currently unsure if we should have the version negotiation in the "version" section; the "content negotiation" section might make more sense. MUST 1.0.0 for within 1.x at least, SHOULD do the highest version, and done?


### ABNF specification

Versioning follows a semantic versioning model on the specification level. Breaking semantic changes (for example removal from ABNF) MUST result in major version bump, however additions MUST be in a minor version bump. Multiple changes may be bundled in the same version update and the version bump will be the biggest version bump in those changes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Versioning follows a semantic versioning model on the specification level. Breaking semantic changes (for example removal from ABNF) MUST result in major version bump, however additions MUST be in a minor version bump. Multiple changes may be bundled in the same version update and the version bump will be the biggest version bump in those changes.
Versioning follows a semantic versioning model on the specification level. Breaking semantic changes MUST be signaled with a major version increase. For example, removing a stanza.
Semantic extensions to the ABNF MUST be signaled with a minor or major version increase. For exmaple, adding a line starting with `# foo`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantic versioning needs a reference in line 38 and following, now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to informative references

Ingestors MUST implement protocol negotiation to be able to support higher than 1.0.0 version.
If exposer doesn't support content negotiation, version 1.0.0 SHOULD be assumed.

An ingestor that supports version "m.y" of the standard MUST support versions "m.x" where "x" and "y" are natural numbers, such that "x < y".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think supporting e.g. 1.0 and 1.5 would be OK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should state this explicitly, because that's not what backwards compatibility means. I would expect that a parser can read anything that lower semantic version (on the same major version).

Comment on lines 319 to 321
Ingestors MUST implement at least version 1.0.0 of the standard.
Ingestors MUST implement protocol negotiation to be able to support higher than 1.0.0 version.
If exposer doesn't support content negotiation, version 1.0.0 SHOULD be assumed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Ingestors MUST implement at least version 1.0.0 of the standard.
Ingestors MUST implement protocol negotiation to be able to support higher than 1.0.0 version.
If exposer doesn't support content negotiation, version 1.0.0 SHOULD be assumed.
Ingestors MUST implement version 1.0.0 of the standard.
Ingestors MAY support versions higher than 1.0.0. In this case, they MUST implement protocol version negotiation.
In case no protocol version negotiation takes place, version 1.0.0 SHOULD be assumed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change the "MUST 1.0.0" to "SHOULD 1.0.0" in 2.x if we need to. It's a bit of a cop-out, but long term, say Prometheus 4.0, it might be defensible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but I'm not sure we'll ever be able to get rid of 1.0.0


An ingestor that supports version "m.y" of the standard MUST support versions "m.x" where "x" and "y" are natural numbers, such that "x < y".

Ingestors MAY refuse to ingest the exposed metrics if content negotiation yields an empty list of versions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems self-evident?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they SHOULD negotiate the latest version as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty academic, as negotiation currently takes place in the exposer, but the alternative would be to try and parse the content as 1.0.0.

I'll remove it as not very useful for now.

specification/OpenMetrics.md Outdated Show resolved Hide resolved
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama marked this pull request as draft September 18, 2024 21:05
@krajorama
Copy link
Member Author

@RichiH I've moved this PR to draft, because of two things:

  1. Now it doesn't say explicitly what the dev summit agreed on: "CONSENSUS: We agree that the understanding of backwards compatibility is that a A.y parser can also parse an A.x format. But the A.x parser doesn’t need to parse the A.y format. Where y > x, A, x, y being integers." In particular if something as backwards compatible, then my originally suggested statement that parser for 1.5 must be able to parse 1.0, 1.1, 1.2, 1.4 as well stands.
  2. Not sure what to do with the specification/draft-richih-opsawg-openmetrics-00.txt and specification/draft-richih-opsawg-openmetrics-01.txt files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants