scrape: provide a fallback format #15136
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
|
🤔 I actually had a quite different idea of how to approach this.
|
|
Maybe we should get some more opinions on this to find out which way to go. @bwplotka you had some thoughts on the original issue, but everyone's feedback welcome. |
|
More in favor of "have a scrape config options, not a command line flag": You might very well have some targets that serve OpenMetrics, but do not return a content-type, while you might have other targets that serve classic Prometheus text format, but do not return a content-type. |
OK, that all makes sense, I'll knock up an alternative approach that is per scrape target (I did play with something like that originally) and takes into account these points. |
|
For the protocol names, are the existing mime types appropriate for use as the values for
|
|
I agree with @beorn7 directions here: #15136 (comment) - this is what I would have in mind too.
What do you mean, why not reusing existing protocol enums in |
Ah ok. I'd been looking at the various existing scrape tests which focused on dealing with the raw Yes, I'll use the same So, for my brain:
v2 does the following:
v3 should do:
The second v3 case (**) is the one where, as suggested above, we could not log anything to mimic v2 behaviour. |
I guess this should be PromParser instead of ProtobufParser. Otherwise, sounds spot on. About the (**) case: I wouldn't worry too much. The logging in v2 is just debug level. I don't think anyone will complain about this changing slightly. The way more impactful change is that we might fail scrapes now that succeeded before, and you need a config change to make them succeed again. And that is the reason why it would be great to get this into the v3 release. |
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Indeed. OK, Revamped to be per scrape target. Will go look for Jan's migration to 3.0 guide as it would be useful to add some comments in there. I did add a new |
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
|
Added a review comment with suggested text to Jan's 3.0 migration PR (#15099 <-- fixed link) |
Signed-off-by: Alex Greenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
Signed-off-by: Alex Greenbank <alex.greenbank@grafana.com>
Signed-off-by: alexgreenbank <alex.greenbank@grafana.com>
OK, all nits addressed, tests rejigged, merged conflicts fixed and all should be good. Almost there! |
beorn7
approved these changes
Oct 18, 2024
KeyOfSpectator
added a commit
to AliyunContainerService/prometheus
that referenced
this pull request
Oct 23, 2024
* master: (667 commits) NHCB scrape: refactor state handling and speed up scrape test (prometheus#15193) feat(tools): add debug printouts to rules unit testing (prometheus#15196) docs: add keep_firing_for in alerting rules api: Add rule group pagination to list rules api (prometheus#14017) Update scrape/scrape.go benchmark, rename parser omtext_with_nhcb goimports run Better docstring on test Remove omcounterdata.txt as redundant Fix failing benchmarks Add unit test to show that current wrapper is sub-optimal Rename convert_classic_histograms to convert_classic_histograms_to_nhcb More followup to prometheus#15164 Follow up prometheus#15178 Followup to prometheus#15164 test(cmd/prometheus): speed up test execution by t.Parallel() when possible feat: normalize "le" and "quantile" labels values upon ingestion scrape: provide a fallback format (prometheus#15136) Disallowing configure AM with the v1 api (prometheus#13883) feat: ProtobufParse.formatOpenMetricsFloat: improve float formatting by using strconv.AppendFloat instead of fmt.Sprint ... # Conflicts: # go.mod # go.sum
Mindavi
added a commit
to Mindavi/nixpkgs
that referenced
this pull request
Dec 2, 2024
…ptions I chose to not constrain them to the supported values to be mo 6D40 re flexible in supporting any newly added values. But maybe it makes sense to constrain them, I'm not completely sure. Ran into this because Prometheus 3.0 was introduced last week and my config broke, since rtl_433 doesn't set the headers / content-type correctly. Related-to: NixOS#358862 Related-to: prometheus/prometheus#15136
This was referenced Dec 2, 2024
Merged
hsjobeki
pushed a commit
to hsjobeki/nixpkgs
that referenced
this pull request
Dec 3, 2024
…ptions I chose to not constrain them to the supported values to be more flexible in supporting any newly added values. But maybe it makes sense to constrain them, I'm not completely sure. Ran into this because Prometheus 3.0 was introduced last week and my config broke, since rtl_433 doesn't set the headers / content-type correctly. Related-to: NixOS#358862 Related-to: prometheus/prometheus#15136
julienduchesne
pushed a commit
to julienduchesne/prometheus
that referenced
this pull request
Dec 13, 2024
|
please add possibility to use it on |
|
Note that this is a fallback for targets that are buggy (i.e. they do not correctly implement content negotiation). We intentionally removed the global default because it turned out to not be safe. Introducing a global option would partially defeat the purpose. If so many of your targets are buggy that you need a global option, I recommend to prioritize fixing the targets. |
stanhu
added a commit
to stanhu/sinatra
that referenced
this pull request
Feb 3, 2025
This commit ensures that all parameters of `Content-Type` are separated with commas (`,`) instead of semicolons (`;`). [RFC 7231](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5) says: ``` Content-Type = media-type Media types are defined in [Section 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1). An example of the field is Content-Type: text/html; charset=ISO-8859-4 ``` [RFC 7231 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1) says: ``` Media types define both a data format and various processing models: how to process that data in accordance with each context in which it is received. media-type = type "/" subtype *( OWS ";" OWS parameter ) type = token subtype = token The type/subtype MAY be followed by parameters in the form of name=value pairs. parameter = token "=" ( token / quoted-string ) The type, subtype, and parameter name tokens are case-insensitive. Parameter values might or might not be case-sensitive, depending on the semantics of the parameter name. The presence or absence of a parameter might be significant to the processing of a media-type, depending on its definition within the media type registry. A parameter value that matches the token production can be transmitted either as a token or within a quoted-string. The quoted and unquoted values are equivalent. For example, the following examples are all equivalent, but the first is preferred for consistency: text/html;charset=utf-8 text/html;charset=UTF-8 Text/HTML;Charset="utf-8" text/html; charset="utf-8" ``` According to [this link](https://stackoverflow.com/a/35879320), it seems that there was prior confusion in early RFCs over this. It appears `,` was a mistake, and that `;` should always be used. Most people probably haven't run into this because in order to trigger this bug: 1. The Sinatra app needs to insert a `;` in the call to `content_type` (such as `content_type "text/plain; version=0.0.4"`). If you omit the `;` in the `Content-Type`, then everything is fine. 2. The client that talks to the app needs to reject `,` in the `Content-Type`. Golang's `mime.ParseMediaType` appears to reject `Content-Type` values that contain `,` but with the introduction of prometheus/prometheus#15136 Prometheus v3 started to fail hard when this occurred. Closes sinatra#2076
stanhu
added a commit
to stanhu/sinatra
that referenced
this pull request
Feb 3, 2025
This commit ensures that all parameters of `Content-Type` are separated with commas (`,`) instead of semicolons (`;`). [RFC 7231](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5) says: ``` Content-Type = media-type Media types are defined in [Section 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1). An example of the field is Content-Type: text/html; charset=ISO-8859-4 ``` [RFC 7231 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1) says: ``` Media types define both a data format and various processing models: how to process that data in accordance with each context in which it is received. media-type = type "/" subtype *( OWS ";" OWS parameter ) type = token subtype = token The type/subtype MAY be followed by parameters in the form of name=value pairs. parameter = token "=" ( token / quoted-string ) The type, subtype, and parameter name tokens are case-insensitive. Parameter values might or might not be case-sensitive, depending on the semantics of the parameter name. The presence or absence of a parameter might be significant to the processing of a media-type, depending on its definition within the media type registry. A parameter value that matches the token production can be transmitted either as a token or within a quoted-string. The quoted and unquoted values are equivalent. For example, the following examples are all equivalent, but the first is preferred for consistency: text/html;charset=utf-8 text/html;charset=UTF-8 Text/HTML;Charset="utf-8" text/html; charset="utf-8" ``` According to [this link](https://stackoverflow.com/a/35879320), it seems that there was prior confusion in early RFCs over this. It appears `,` was a mistake, and that `;` should always be used. Most people probably haven't run into this because in order to trigger this bug: 1. The Sinatra app needs to insert a `;` in the call to `content_type` (such as `content_type "text/plain; version=0.0.4"`). If you omit the `;` in the `Content-Type`, then everything is fine. 2. The client that talks to the app needs to reject `,` in the `Content-Type`. Golang's `mime.ParseMediaType` appears to reject `Content-Type` values that contain `,` but with the introduction of prometheus/prometheus#15136 Prometheus v3 started to fail hard when this occurred. Closes sinatra#2076
stanhu
added a commit
to stanhu/sinatra
that referenced
this pull request
Feb 3, 2025
This commit ensures that all parameters of `Content-Type` are separated with commas (`,`) instead of semicolons (`;`). [RFC 7231](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5) says: ``` Content-Type = media-type Media types are defined in [Section 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1). An example of the field is Content-Type: text/html; charset=ISO-8859-4 ``` [RFC 7231 3.1.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1) says: ``` Media types define both a data format and various processing models: how to process that data in accordance with each context in which it is received. media-type = type "/" subtype *( OWS ";" OWS parameter ) type = token subtype = token The type/subtype MAY be followed by parameters in the form of name=value pairs. parameter = token "=" ( token / quoted-string ) The type, subtype, and parameter name tokens are case-insensitive. Parameter values might or might not be case-sensitive, depending on the semantics of the parameter name. The presence or absence of a parameter might be significant to the processing of a media-type, depending on its definition within the media type registry. A parameter value that matches the token production can be transmitted either as a token or within a quoted-string. The quoted and unquoted values are equivalent. For example, the following examples are all equivalent, but the first is preferred for consistency: text/html;charset=utf-8 text/html;charset=UTF-8 Text/HTML;Charset="utf-8" text/html; charset="utf-8" ``` According to [this link](https://stackoverflow.com/a/35879320), it seems that there was prior confusion in early RFCs over this. It appears `,` was a mistake, and that `;` should always be used. Most people probably haven't run into this because in order to trigger this bug: 1. The Sinatra app needs to insert a `;` in the call to `content_type` (such as `content_type "text/plain; version=0.0.4"`). If you omit the `;` in the `Content-Type`, then everything is fine. 2. The client that talks to the app needs to reject `,` in the `Content-Type`. Golang's `mime.ParseMediaType` appears to reject `Content-Type` values that contain `,` but with the introduction of prometheus/prometheus#15136 Prometheus v3 started to fail hard when this occurred. Closes sinatra#2076
dentarg
pushed a commit
to sinatra/sinatra
that referenced
this pull request
Feb 9, 2025
This commit ensures that all parameters of `Content-Type` are separated with commas (`,`) instead of semicolons (`;`). RFC 7231 (https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5) says: Content-Type = media-type Media types are defined in Section 3.1.1.1. An example of the field is Content-Type: text/html; charset=ISO-8859-4 RFC 7231 3.1.1.1 (https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.1) says: Media types define both a data format and various processing models: how to process that data in accordance with each context in which it is received. media-type = type "/" subtype *( OWS ";" OWS parameter ) type = token subtype = token The type/subtype MAY be followed by parameters in the form of name=value pairs. parameter = token "=" ( token / quoted-string ) The type, subtype, and parameter name tokens are case-insensitive. Parameter values might or might not be case-sensitive, depending on the semantics of the parameter name. The presence or absence of a parameter might be significant to the processing of a media-type, depending on its definition within the media type registry. A parameter value that matches the token production can be transmitted either as a token or within a quoted-string. The quoted and unquoted values are equivalent. For example, the following examples are all equivalent, but the first is preferred for consistency: text/html;charset=utf-8 text/html;charset=UTF-8 Text/HTML;Charset="utf-8" text/html; charset="utf-8" According to https://stackoverflow.com/a/35879320, it seems that there was prior confusion in early RFCs over this. It appears `,` was a mistake, and that `;` should always be used. Most people probably haven't run into this because in order to trigger this bug: 1. The Sinatra app needs to insert a `;` in the call to `content_type` (such as `content_type "text/plain; version=0.0.4"`). If you omit the `;` in the `Content-Type`, then everything is fine. 2. The client that talks to the app needs to reject `,` in the `Content-Type`. Golang's `mime.ParseMediaType` appears to reject `Content-Type` values that contain `,` but with the introduction of prometheus/prometheus#15136 Prometheus v3 started to fail hard when this occurred.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #13214
Provides a per scrape target setting for a fallback format for scraping that is used if the received
Content-Typedoes not match an expected value, or is missing/blank.This is a breaking change as some scrapes that worked with Prometheus 2.x may now require the
fallback_scrape_protocolbeing set in order to force the scrape.TODO