[prometheus] Update the error check to handle invalid and empty content header #47085

devamanv · 2025-10-14T12:52:24Z

Proposed commit message

The PR contains changes to improve the handling the issue that would cause Metricbeat Prometheus module to not fetch any metrics in case of invalid and empty content type header.

Note: Previously it was relying on a particular upstream change. This replaces it with better handling.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Prometheus metricbeat module doesn't fetch metrics when content type is blank #47033

github-actions · 2025-10-14T12:52:37Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

mergify · 2025-10-14T12:53:01Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @devamanv? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

elasticmachine · 2025-10-14T13:02:27Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

ishleenk17 · 2025-10-14T13:34:51Z

metricbeat/helper/prometheus/textparse_test.go

 	}
 	require.ElementsMatch(t, expected, result)
 }



We have UT's for no header, invalid header now.
Lets have one for valid header too.
Just to have all the coverage.

tommyers-elastic

LGTM thanks. we are missing the changelog though

tommyers-elastic · 2025-10-14T13:37:17Z

and i agree with ishleen on the unit tests - let's add as many test cases as we can think of

@lalit-satapathy had some additional cases he identified that would make good test inputs.

ishleenk17 · 2025-10-14T13:44:40Z

and i agree with ishleen on the unit tests - let's add as many test cases as we can think of

@lalit-satapathy had some additional cases he identified that would make good test inputs.

Yeah, Lalit had one for Content-Type: text/html; charset=utf-8 header. Although that would come into invalid type as per Prometheus header support. But surely we can add it.

…-fix

devamanv · 2025-10-15T05:38:09Z

@lalit-satapathy had some additional cases he identified that would make good test inputs.

@tommyers-elastic I have added tests for these additional cases here. Could you please review it again?

ishleenk17 · 2025-10-15T05:45:02Z

metricbeat/helper/prometheus/textparse_test.go

+		{"html", "text/html; charset=utf-8", false},
+		{"empty_content_type", "", false},
+		{"openmetrics", "application/openmetrics-text", false},
+		{"octet_stream", "application/octet-stream", false},


Lets add one for protobuf too: Should behave the same way
Content-Type: application/vnd.google.protobuf

lalit-satapathy · 2025-10-15T05:47:08Z

Also please add this case:

Content-Type: text/plain; version=1.0.0; charset=utf-8; charset=utf-8

devamanv · 2025-10-15T09:54:54Z

could someone from @elastic/elastic-agent-data-plane please review this as well?

rdner

One question to clarify.

rdner · 2025-10-15T10:25:03Z

metricbeat/helper/prometheus/textparse.go

-	// This check allows to continue where the content type is blank but the parser is non-nil. Returns error on all other cases.
-	if err != nil && !strings.Contains(err.Error(), "non-compliant scrape target sending blank Content-Type, using fallback_scrape_protocol") {
+	// This check allows to continue where the content type is blank/invalid but the parser is non-nil. Returns error on all other cases.
+	if parser == nil {


How confident are we that textparse.New will always return err != nil when parser == nil?

Looks risky to me, I assume that consumers of this function would not expect return nil, nil from here.

Looks like it all relies on the fact that these 2 functions are in sync:

https://github.com/prometheus/prometheus/blob/fd421dc3b1cf542108e1328623e38b745cbd3f49/model/textparse/interface.go#L115

https://github.com/prometheus/prometheus/blob/fd421dc3b1cf542108e1328623e38b745cbd3f49/model/textparse/interface.go#L164-L176

I think it's totally possible that at some point someone adds a new content type support to the extractMediaType and then forgets to update the switch in the New function and then the default switch case returns nil, nil.

In this case we also return nil, nil from this condition.

this is a weird quirk of this function that it returns valid parsers when err != nil.

there's not really a great way to handle it here. since we know valid parsers get returned with errors, we can't rely on the error to gate if we should fail. the only way is to check the parser.

i think what's here is the best we can do. if the function ends up returning nil, nil, it's still an error condition for us since there's no parser.

maybe we wrap the error that we return here to clarify this?

return nil, fmt.Errorf("nil parser returned from textparse.New: %w", err)

@devamanv

+ if parser == nil { + if err != nil { + return nil, err + } + return nil, fmt.Errorf("no parser returned for contentType %q (err = %v)", contentType, err) + }

Lets add this patch

return nil, fmt.Errorf("no parser returned for contentType %q (err = %v)", contentType, err)

@ishleenk17 err is always nil on this line. No need to include it into the string.

Update the error check to handle invalid and empty content header

39663af

devamanv self-assigned this Oct 14, 2025

devamanv added the bugfix label Oct 14, 2025

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 14, 2025

devamanv marked this pull request as ready for review October 14, 2025 12:56

devamanv requested a review from a team as a code owner October 14, 2025 12:56

devamanv requested review from VihasMakwana and andrzej-stencel October 14, 2025 12:57

pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 14, 2025

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 14, 2025

pierrehilbert requested review from rdner and removed request for andrzej-stencel October 14, 2025 13:02

ishleenk17 reviewed Oct 14, 2025

View reviewed changes

tommyers-elastic approved these changes Oct 14, 2025

View reviewed changes

mykola-elastic approved these changes Oct 15, 2025

View reviewed changes

devamanv added 2 commits October 15, 2025 11:04

Add unit tests for a set of possible media types and logging for error

8eceaaf

Merge remote-tracking branch 'upstream/main' into content-type-header…

2940818

…-fix

ishleenk17 reviewed Oct 15, 2025

View reviewed changes

devamanv added 2 commits October 15, 2025 11:22

Add changelog fragment for the content type handling change

eef7619

Add a test case for text plain version 1.0.0

2c595d5

ishleenk17 approved these changes Oct 15, 2025

View reviewed changes

rdner reviewed Oct 15, 2025

View reviewed changes

[prometheus] Update the error check to handle invalid and empty content header #47085

Are you sure you want to change the base?

[prometheus] Update the error check to handle invalid and empty content header #47085

Conversation

devamanv commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Uh oh!

github-actions bot commented Oct 14, 2025

🤖 GitHub comments

Uh oh!

mergify bot commented Oct 14, 2025

Uh oh!

elasticmachine commented Oct 14, 2025

Uh oh!

ishleenk17 Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

tommyers-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

tommyers-elastic commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishleenk17 commented Oct 14, 2025

Uh oh!

devamanv commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishleenk17 Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

lalit-satapathy commented Oct 15, 2025

Uh oh!

devamanv commented Oct 15, 2025

Uh oh!

rdner left a comment

Choose a reason for hiding this comment

Uh oh!

rdner Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

rdner Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

tommyers-elastic Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

ishleenk17 Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdner Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

devamanv commented Oct 14, 2025 •

edited

Loading

tommyers-elastic commented Oct 14, 2025 •

edited

Loading

devamanv commented Oct 15, 2025 •

edited

Loading

ishleenk17 Oct 15, 2025 •

edited

Loading

rdner Oct 15, 2025 •

edited

Loading