-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement automatic exposition format detection #14445
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
The |
Label |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
It's complaining about a license year due to file renames. I think it's safe to ignore that validation error on this instance; it's a bit of an edge case for the license header validation. |
@property | ||
def parse_metric_families(self): | ||
media_type = self._content_type.split(';')[0] | ||
return self._parsers[media_type] |
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.
Based on https://github.com/prometheus/prometheus/blob/v2.43.0/model/textparse/interface.go#L83-L90 couldn't we get rid of the parser mapping attribute and this could be simplified?
return self._parsers[media_type] | |
return ( | |
parse_openmetrics | |
if self.use_latest_spec or media_type == 'application/openmetrics-text' | |
else parse_prometheus | |
) |
70d3df2
to
6121ec6
Compare
The |
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.
Awesome!
In preparation for adding tests for dynamic parsing based on content type
Instead of "strict"
6121ec6
to
1abfce5
Compare
The |
* Put all tests for OpenMetrics V2 in the same file In preparation for adding tests for dynamic parsing based on content type * Rename test file * Choose openmetrics parser dynamically * Add content-type to prometheus test * Refactor fixtures into more granular ones * Rename fixtures to use prometheus / openmetrics Instead of "strict" * Test that `use_latest_spec` behaves as expected regardless of `Content-Type` * Rename parsing function names * Small refactor in parser selection * Move parser selection to explicit conditional 7d8dc6d
As those are no longer needed after #14445.
* Be specific about the exposition formats supported * Remove references to configuration of actual format As those are no longer needed after #14445.
What does this PR do?
Changes the decision of what parser to use for OpenMetrics (Prometheus or OpenMetrics) to be dynamic, based on the
Content-Type
header.Motivation
AI-2416.
The decision to use the OpenMetrics parser (a.k.a. strict in the code) or the Prometheus format parser was based on a config option called
use_latest_spec
, which is not very obvious. As it so happens, both formats specify what theContent-Type
header should look like when using each of those formats, which makes it reasonable for us to rely on it to make that decision dynamically instead of by using a explicit option. This will also keep our integrations working without changes in the event existing applications make the switch from the currently more widespread prometheus format to the OpenMetrics standard (which is supposed to represent the future). It should safe to rely on that header, as Prometheus itself does the same.Additional Notes
use_latest_spec
still exists as a escape hatch to force theOpenMetrics
format regardless of content header. A follow-up PR will hide it.use_latest_spec
option #14446 into this in order to apply the same versioning change to all affected integrations.Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attachedqa/skip-qa
label.