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

[Vert.x 4] Incorrect HTTP metric reporting #15593

Closed
cescoffier opened this issue Mar 10, 2021 · 6 comments
Closed

[Vert.x 4] Incorrect HTTP metric reporting #15593

cescoffier opened this issue Mar 10, 2021 · 6 comments
Assignees
Labels
Milestone

Comments

@cescoffier
Copy link
Member

Describe the bug

An assertion from io.quarkus.it.micrometer.prometheus.PrometheusMetricsRegistryTest does not pass with Vert.x 4.

With an endpoint like:

@GET
@Path("item/{id}")
public String item(@PathParam("id") String id) {
    return "return message with id " + id;
}

The reported metrics uses the actual path instead of the parameterized path:

http_server_requests_seconds_count{env="test",method="GET",outcome="SUCCESS",registry="prometheus",status="200",uri="/message/item/123",} 1.0
http_server_requests_seconds_sum{env="test",method="GET",outcome="SUCCESS",registry="prometheus",status="200",uri="/message/item/123",} 0.002675179

This will lead to OOM as for each unique id, a metric will be captured.

Expected behavior

It should report:

http_server_requests_seconds_count{env="test",method="GET",outcome="SUCCESS",registry="prometheus",status="200",uri="/message/item/{id}",} 1.0
http_server_requests_seconds_sum{env="test",method="GET",outcome="SUCCESS",registry="prometheus",status="200",uri="/message/item/{id}",} 0.002675179

To Reproduce

In the vertx-snapshot branch:

  • Open io.quarkus.it.micrometer.prometheus.PrometheusMetricsRegistryTest
  • Uncomment line 137
  • Run -> boom

Note: The assertion has been commented to avoid being stuck. However, this issue should be considered critical.

@cescoffier cescoffier added the kind/bug Something isn't working label Mar 10, 2021
@cescoffier cescoffier added this to the 2.x milestone Mar 10, 2021
@cescoffier
Copy link
Member Author

I was able to workaround the issue by specifying an explicit match pattern (/message/item/\[0-9]+=/message/item/{id}).

cescoffier added a commit that referenced this issue Mar 10, 2021
@ebullient
Copy link
Member

This should be related to #15047 , but could also require micrometer to change how it is storing state against Vertx context.

@cescoffier
Copy link
Member Author

@ebullient I've seen work around that. Are we good?

@ebullient
Copy link
Member

Not quite. I put a lid on it (no OOM), if you want to try that and call that an answer to this issue..

Better recognition of patterns for client / server / etc. is still in flight.

@cescoffier
Copy link
Member Author

@ebullient should this one be closed?

@ebullient
Copy link
Member

Close as duplicate of #15231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants