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

JSON-Binding breaks prometheus metrics format #645

Closed
tomas-langer opened this issue May 3, 2019 · 7 comments · Fixed by #669
Closed

JSON-Binding breaks prometheus metrics format #645

tomas-langer opened this issue May 3, 2019 · 7 comments · Fixed by #669
Assignees
Labels
bug Something isn't working P1 SE webserver

Comments

@tomas-langer
Copy link
Member

When I enable JsonBindingSupport for the webserver routing and MetricsSupport:

Routing.builder()
 .register(JsonBindingSupport.create())
 .register(MetricsSupport.create())
 .build()

The clear-text Prometheus format is broken:

curl http://localhost:8080/metrics/vendor
"# TYPE vendor:requests_count counter\n# HELP vendor:requests_count Each request (regardless of HTTP method) will increase this counter\nvendor:requests_count 2\n# TYPE vendor:requests_meter_total counter\n# HELP vendor:requests_meter_total Each request will mark the meter to see overall throughput\nvendor:requests_meter_total 2\n# TYPE vendor:requests_meter_rate_per_second gauge\nvendor:requests_meter_rate_per_second 0.3750436866122316\n# TYPE vendor:requests_meter_one_min_rate_per_second gauge\nvendor:requests_meter_one_min_rate_per_second 0.2\n# TYPE vendor:requests_meter_five_min_rate_per_second gauge\nvendor:requests_meter_five_min_rate_per_second 0.2\n# TYPE vendor:requests_meter_fifteen_min_rate_per_second gauge\nvendor:requests_meter_fifteen_min_rate_per_second 0.2\n"%

Expected output:

curl http://localhost:8080/metrics/vendor
# TYPE vendor:requests_count counter
# HELP vendor:requests_count Each request (regardless of HTTP method) will increase this counter
vendor:requests_count 1
# TYPE vendor:requests_meter_total counter
# HELP vendor:requests_meter_total Each request will mark the meter to see overall throughput
vendor:requests_meter_total 1
# TYPE vendor:requests_meter_rate_per_second gauge
vendor:requests_meter_rate_per_second 0.21839295518592852
# TYPE vendor:requests_meter_one_min_rate_per_second gauge
vendor:requests_meter_one_min_rate_per_second 0.0
# TYPE vendor:requests_meter_five_min_rate_per_second gauge
vendor:requests_meter_five_min_rate_per_second 0.0
# TYPE vendor:requests_meter_fifteen_min_rate_per_second gauge
vendor:requests_meter_fifteen_min_rate_per_second 0.0

Environment Details

  • Helidon Version: 1.0.3, 1.0.4-SNAPSHOT
  • Helidon SE
  • JDK version: 1.8
  • OS: MacOS

@tomas-langer tomas-langer added bug Something isn't working SE webserver labels May 3, 2019
@tomas-langer
Copy link
Member Author

The string is wrapped in quotes and end of line characters are removed.
In this case the string should not be touched by JsonBindingSupport

@ljnelson
Copy link
Member

ljnelson commented May 3, 2019

Hi, @tomas-langer; should this be special-cased? As you can see from the code there's nothing special going on. You added JsonBindingSupport, so it simply uses the writer created:

https://github.com/oracle/helidon/blob/8454a8d7fe07ae2ce91769b3a61a6c8e3027baa0/media/jsonb/common/src/main/java/io/helidon/media/jsonb/common/JsonBinding.java#L85

Should there be some way for JsonBindingSupport to know that it is not supposed to do this, even though it was registered? Or did you have something else in mind?

@barchetta
Copy link
Member

Maybe a quick fix would be to change JsonBindingSupport so it does not register the writer if payload instanceof String?

But we do have a general problem here. Metrics (and Health and others) can't know or care about what media support the user has registered. And the media support the user has registered should not impact these built-in services (since they likely hand craft their responses).

So we either need a way to suppress the media support per response, or do something like having specific entity/writers for these built in services so the selection of the appropriate writer is unambiguous. Or maybe a RawPayload entity that wraps a string and indicates media specific content writers should keep their hands off.

@tomas-langer
Copy link
Member Author

My first solution would be to ignore String. The RawPayload suggested by Joe seems like a good approach as well.
Maybe a workaround for the RawPayload would be to use Publisher<DataChunk> in these services , as I guess that gets around the writers as well?

@barchetta
Copy link
Member

barchetta commented May 9, 2019

The bookstore functional test did not catch this since it only checks metrics with the JSON-P support configured. I'll update the test to run with JSON-B and Jackson as well.

barchetta added a commit to barchetta/helidon that referenced this issue May 9, 2019
@barchetta barchetta assigned barchetta and unassigned ljnelson May 9, 2019
@barchetta barchetta added the P1 label May 9, 2019
@barchetta
Copy link
Member

JacksonSupport has the same problem.

@barchetta
Copy link
Member

See PR #669

barchetta added a commit that referenced this issue May 10, 2019
* Fix for #645 JSON-Binding breaks prometheus metrics format
@m0mus m0mus added this to Backlog Aug 12, 2024
@m0mus m0mus moved this to Closed in Backlog Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1 SE webserver
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants