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

Prometheus fails to scrape Helidon metrics #111

Closed
barchetta opened this issue Oct 16, 2018 · 6 comments
Closed

Prometheus fails to scrape Helidon metrics #111

barchetta opened this issue Oct 16, 2018 · 6 comments
Assignees
Labels
bug Something isn't working
Projects

Comments

@barchetta
Copy link
Member

Environment Details

  • Helidon Version: 0.10.1
  • Helidon SE
  • JDK version: 1.8.0_162
  • OS: MacOS

Problem Description

Prometheus 2.2.1 and 2.4.3 fail to scrape Helidon metrics. Prometheus reports the error no token found. This error is typically reported by Prometheus when it can't parse the scrapped metrics data.

Steps to reproduce

  1. Build and run this Helidon app: https://github.com/barchetta/helidon-se-codeone-2018
  2. Download and run Prometheus 2.2.1 (https://github.com/prometheus/prometheus/releases/tag/v2.2.1)
  3. Add a scrape_configs to prometheus.yml like this:
  - job_name: 'helidon'

    metrics_path: '/metrics/'
    # metrics_path defaults to '/metrics'
    # scheme defaults to 'http'.

    static_configs:
      - targets: ['localhost:8080']

You'll see prometheus report:

level=warn ts=2018-10-16T01:35:44.714172501Z caller=scrape.go:697 component="scrape manager" scrape_pool=helidon target=http://localhost:8080/metrics/ msg="append failed" err="no token found"
@barchetta
Copy link
Member Author

The issue appears to be that Prometheus sends an Accept header that looks like:

text/plain;version=0.0.4;q=1,*/*;q=0.1

So it is requesting text/plain with a quality of 1, but will also accept anything. One can argue this isn't correct (since Prometheus can't handle JSON for example). But that's not the point.

The point is that Helidon will generate JSON in this case, and not Prometheus text. This is because of this method:

    private void getMultiple(ServerRequest req, ServerResponse res, Registry... registries) {
        if (req.headers().isAccepted(MediaType.APPLICATION_JSON)) {
            res.send(toJsonData(registries));
        } else {
            res.send(toPrometheusData(registries));
        }
    }

Since the request headers will accept JSON, we generate JSON.

The logic needs to be more sophisticated. Since Prometheus prefers text/plain, that's what we should generate. To be explicit:

If the Accept header is text/plain;version=0.0.4;q=1,*/*;q=0.1 then we should generate a Prometheus text report.

If the Accept header is application/json;version=0.0.4;q=1,*/*;q=0.1 then we should generate a JSON report.

@barchetta barchetta added the bug Something isn't working label Oct 16, 2018
@spericas
Copy link
Member

Interesting, I agree. Take a look at HashRequestHeaders::bestAccepted.

@barchetta barchetta assigned barchetta and unassigned tomas-langer Oct 17, 2018
@barchetta barchetta added this to In Review in Team Board Oct 17, 2018
@tomas-langer
Copy link
Member

I have requested clarification from Metrics spec on media types:
https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/microprofile/t4YgzPpmOsA

My current understanding of the spec would mean that we cannot change the behavior as requested by this issue.

@barchetta
Copy link
Member Author

Thanks for asking for clarification. I hope this is a case of the MicroProfile spec being sloppy because the current behavior means that MicroProfile apps (and by extension Helidon apps) can't be scraped by Prometheus -- which is absurd.

@spericas
Copy link
Member

@barchetta I agree. The spec may need to be clarified, but I doubt their intent was to ignore quality factors in an Accept header. We should really return text/plain for that value of Accept.

@barchetta
Copy link
Member Author

We have confirmation from MicroProfile that the spec is misleading:
https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/microprofile/t4YgzPpmOsA

"I'm part of the metrics community and I agree that what's specified in section 2.3, related to the Accept header isn't as specific as it ought to be. The way it's worded in the spec right now isn't right, as illustrated by Joe's example.

The way we'd intended for it to be interpreted was that if the incoming request accepts text/plain with a higher priority than application/json, then you should use Prometheus format. If the incoming request accepts application/json with a higher priority than text/plain, then use JSON format. If the incoming request doesn't accept either of those, then return a 406 status code."

There is now an issue to clarify this in the spec: eclipse/microprofile-metrics#291

@barchetta barchetta moved this from In Review to Done in Team Board Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Team Board
  
Done
Development

No branches or pull requests

3 participants