accept text/plain type by default for prometheus client scraping

[newly12]

What does this PR do?

accept text/plain type by default for prometheus client scraping

Why is it important?

prometheus java client 0.10.0 introduced breaking changes that when application/openmetrics-text is seen in accept list, new types(i.e. info) will be exposed and they are not supported by current metrics parser

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.

Author's Checklist

• [ ]

How to test this PR locally

• start a java app instrumented with prometheus client version 0.10.0 and then start metricbeat to scrape it

Related issues

• Closes new openmetrics types are not supported for prometheus module #24554

Use cases

Screenshots

Logs

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: jsoriano commented: /test

• Start Time: 2021-03-24T12:50:11.264+0000

• Duration: 63 min 23 sec

• Commit: 307060b

Test stats 🧪

Test	Results
Failed	0
Passed	8899
Skipped	2385
Total	11284

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	8899
Skipped	2385
Total	11284

[elasticmachine]

Pinging @elastic/integrations (Team:Integrations)

[jsoriano]

Thanks for reporting this!

they are not supported by current metrics parser

Does it produce failures in Metricbeat?

[jsoriano]

Metricbeat has two collectors, one for prometheus, and another one for openmetrics, they are mostly the same so far, but I think it would make sense if each collector configured the default headers of their choice.

[newly12]

Thanks for the comments. yeah but I think for application/openmetrics-text the parser would only work for 'previous' openmetrics format, not the new one, so sending Accept: application/openmetrics-text could give target endpoints the chance to respond with new format, while at this moment, text/plain prometheus format should still be compatible from their perspective.

This PR is more like a workaround as being mentioned in the issue, the solution would still be updating the parser as suggested by @exekias .

[ChrsMark]

I wonder why application/openmetrics-text; version=0.0.1 includes the new openmetrics types 🤔 . Shouldn't that change be applied with a new version so as to be backwards compatible?

[newly12]

good point. I think somehow that is not happening and hence breaking things.

[jsoriano]

cc @ChrsMark @exekias

[jsoriano]

/test

[ChrsMark]

Looking at #17291 this header was added by Blake to work in general for all cases since these are the headers used by prometheus upstream too.

In the issue that Blake tried to fix (#16870) I found a config that helps the user and might be helpful in this case too: #16870 (comment).

@newly12 do you think sth like the following could work in your case?

- module: prometheus
  period: 10s
  hosts: ["localhost:80"]
  metrics_path: /platform/prometheus
  headers:
    accept: "`text/plain;version=0.0.4;q=0.5,*/*;q=0.1"

[newly12]

thanks @ChrsMark , it should work by setting overridden header in config. concern is that this needs to be removed after the long term fix done in metricbeat and before prom text compatible removed(if they will) in various sdk. I would assume long term fix requires parser update(i.e. move to textparse?) and maybe effort for new types mapping, which sounds like may not happen in a short period of time..

[ChrsMark]

@newly12 I will create a separate issue to track the investigation of the updates that are required (I will check textparse thnaks!). Do you think it's ok to close this one (along with the original issue) if overriding the header works for you? Then we can continue the chat at the follow up issue (#24707).

[vjsamuel]

@ChrsMark this is an issue everytime someone tries to report a metric of type info. especially when we use hints based autodiscover we cant keep setting headers for each use case. given that we dont natively support open metrics scraping at this time, how can we set that as the default header? we should not use it, unless we move to textparse which is the only native openmetrics parser at the moment. expfmt is more or less deprecated.

[ChrsMark]

Makes sense @vjsamuel , thanks for reporting this. I think we can proceed with this PR and merge it since as described at #24707 (comment) adding application/openmetrics-text does not change anything in the process.

@jsoriano @exekias what do you think about merging this one?

[jsoriano]

Sounds good to me.

[exekias]

I'm ok with patching this way for now. One question on my end, where is openmetricsHeader used right now?

[ChrsMark]

I'm ok with patching this way for now. One question on my end, where is openmetricsHeader used right now?

Nowhere, we can remove it.

[ChrsMark]

Let's proceed with merging this one after we cleanup the unwanted headers.

[newly12]

Thanks all! I've removed unnecessary openmetricsHeader.

[jsoriano]

/test

[ChrsMark]

lgtm, thanks @newly12 for your effort here!