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

The path tag on HTTP server request metrics moved from using path() to uri() (from the HTTP request). #225

Closed
ppatierno opened this issue Aug 5, 2024 · 25 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ppatierno
Copy link
Member

Hi,
we use the Vert.x micrometer metrics component with our Strimzi HTTP bridge in order to get (out of box) metrics around HTTP server related metrics.
We noticed this issue strimzi/strimzi-kafka-bridge#921 and as you can see from my last comment there, it seems that it was a change in Vert.x long time ago we didn't notice.
The current path tag on the HTTP server request metrics is using the request.uri() since Vert.x 4.0.0, while it was using request.path() before. This implies that the tag contains the full URI with the host address and also the query string while what we expect is just the HTTP path as it was before.
Is there any specific reasons why it was changed this way (because it's also broken our dashboards).
I could already have a workaround by using a custom tag provider to set the path tag as we expect to revert back the old behaviour but I am curious about the rationale behind that change.

@ppatierno
Copy link
Member Author

@jotak I see you made the change with this commit f19fe62 can you clarify it please? Because right now on our side the only way I have is by using a custom tag provider overriding the path tag with what it was before/expected.

@ppatierno
Copy link
Member Author

ppatierno commented Aug 5, 2024

I have a feeling ... I see that in the same commit you moved from HttpServerRequest which exposes path() to HttpRequest which doesn't. Maybe it was just a move to use uri() for this reason but not taking into account the implications of that?

@jotak
Copy link
Contributor

jotak commented Aug 5, 2024

Hi @ppatierno
It's a 4 years old commit that we are talking about, so I can't guarantee to remember everything in details.
But if I'm correct, this was when switching from vert.x 3 to 4, correct? There was a few breaking changes in the metrics API that required changes like this one; cf eclipse-vertx/vert.x@c1eb999
As you suspect, the API changed to use HttpRequest instead of HttpServerRequest, which was not a transparent replacement. Since moving from vert.x 3 to 4 permits breaking changes, it was considered ok.

cc @vietj if you have anything to add

@ppatierno
Copy link
Member Author

Hi @jotak, first of all thanks for your fast reply.

There was a few breaking changes in the metrics API that required changes like this one; cf eclipse-vertx/vert.x@c1eb999
As you suspect, the API changed to use HttpRequest instead of HttpServerRequest, which was not a transparent replacement

Yeah I saw that change, and tbh I didn't understand the purpose of moving from HttpRequest to HttpServerRequest. The HttpServerMetrics class where you have the requestBegin for example (togather with all the other methods) is specific for the server metrics, so leaving HttpServerRequest should have worked fine, or? But I am not that much into Vert.x core and metrics related part so I will leave to you the explanation.

Since moving from vert.x 3 to 4 permits breaking changes, it was considered ok.

Well tbh it could be considered ok in terms of breaking API but here we are breaking the meaning of a tag referring to an HTTP concept which is the path. It's the resource path, it should not contain host or query string. So even if breaking changes were allowed, this one has more bad implications than just breaking API. Also can you imagine the magnitude of metrics generated this way? The same calls to the same resource path but having different values for query parameters are accumulated as separated metrics. Usually query parameters don't make any good difference, most of the times you are interested in the called resource path as it was working well in Vert.x 3.x.

I am interested to know what @vietj thinks about that, but right now I see a bad outcome for the Vert.x users leveraging the HTTP server metrics. On our side we made a mistake not noticing that since long time :-(

@jotak
Copy link
Contributor

jotak commented Aug 6, 2024

@ppatierno I'm not involved in vert.x metrics anymore and I might be off / but as far as I remember, this label is not enabled by default, or is it? If it's enabled by default, it should certainly be changed. cf doc here: https://vertx.tk/docs/vertx-micrometer-metrics/js/#_labels_and_matchers :

Warning: Enabling labels may result in a high cardinality in values, which can cause troubles on the metrics backend and affect performances. So it must be used with care. In general, it is fine to enable labels when the set of possible values is bounded.

So what you say about cardinality is true, but is clearly stated in the doc, and I hope that risky path is disabled by default. It makes sense to turn this label on in non-public facing api if you know that values will be bounded. In the opposite case, iirc the custom tag provider would be the way to go, like you mentioned above. There's also an example in the doc (look under "Using Micrometer’s MeterFilter") that might be what you want.

I agree that this particular point was handled in a more straightforward way in vert.x 3, and unfortunately I don't remember all the context, probably @vietj could, but I'm pretty sure there was a good rationale for doing that API change which made the trade-off worth it.

@ppatierno
Copy link
Member Author

Yes path is not enabled by default but the previous code didn't have the high risk of high metrics cardinality as today. It was just the HTTP resource path nothing more, no host, no query string (and the query parameters are the high risk of an high cardinality).

It makes sense to turn this label on in non-public facing api if you know that values will be bounded

Well this makes the label almost useless if restrict to some specific use cases. The bigger problem of the high cardinality now is related to the query string, how many parameters you can have and how many possible values for them. As said, it makes the path label totally useless then, while before it was really useful and right in terms of meaning (again).

My gut feeling is that the first implementation was right by using the path() method then, when the refactoring ended by having the HttpRequest not exposing path() but only uri() the only solution was using the last one, but not taking into account implications.

Let's wait for @vietj, maybe something is still possible to revert the original meaning back.

@jotak
Copy link
Contributor

jotak commented Aug 6, 2024

I'm pretty sure you can keep using path, and by adding a MeterFilter you can retrieve the same behaviour as before

@ppatierno
Copy link
Member Author

Right now our workaround was applying a custom tags provider overriding the path this way

.setServerRequestTagsProvider(req -> {
                    String path = null;
                    if (req instanceof Http1xServerRequest) {
                        path = ((Http1xServerRequest) req).path();
                    } else if (req instanceof Http2ServerRequest) {
                        path = ((Http2ServerRequest) req).path();
                    }
                    return path != null ? List.of(Tag.of(Label.HTTP_PATH.toString(), path)) : List.of();
                });

but maybe MeterFilter could be a better solution. Could me point to some examples of its usage?

@ppatierno
Copy link
Member Author

I guess something around using MeterFilter.replaceTagValues could help, or?

@ppatierno
Copy link
Member Author

Let's wait for @vietj, maybe something is still possible to revert the original meaning back.

@vietj can you provide your insights about this issue, please?

@ppatierno
Copy link
Member Author

@vietj any news? We should move forward on our Strimzi bridge project and make a decision related to ... your decision here :-) If it's not going to be fixed in Vert.x we need to apply a workaround on our side.

@vietj
Copy link
Contributor

vietj commented Oct 4, 2024

@ppatierno I'ill have a loop asap

@vietj
Copy link
Contributor

vietj commented Oct 4, 2024

I think vertx-micrometer-metrics should extract the path from the value returned by uri() to correctly report the tag

@ppatierno
Copy link
Member Author

I think vertx-micrometer-metrics should extract the path from the value returned by uri() to correctly report the tag

Just to be sure, it sounds to me that vertx-micrometer-metrics needs a fix then. Does this component have a maintainer to ask for it?

@vietj
Copy link
Contributor

vietj commented Oct 7, 2024

@ppatierno yes it needs a fix indeed, there is no official maintainer atm, @tsegismont and myself are taking care of it

@vietj vietj added this to the 4.5.11 milestone Oct 7, 2024
@vietj
Copy link
Contributor

vietj commented Oct 7, 2024

I scoped this issue to the next 4.5.x release @ppatierno

@tsegismont tsegismont self-assigned this Oct 7, 2024
@ppatierno
Copy link
Member Author

@vietj @tsegismont thank you very much for taking care of this!

tsegismont added a commit to tsegismont/vertx-micrometer-metrics that referenced this issue Oct 17, 2024
@tsegismont
Copy link
Contributor

@vietj @ppatierno I've sent #240

@vietj if we backport this to 4.x, we must clearly indicate the change in the breaking changes documentation (the metric tags are going to be different).

I believe it's acceptable given this tag is disabled by default and there's a workaround for users who want the current behavior (which, again, is not consistent).

@vietj
Copy link
Contributor

vietj commented Oct 18, 2024

in 4.x can't we have both tags @tsegismont

@tsegismont
Copy link
Contributor

It's just one tag: PATH, with a different value.

Are you suggesting to create a separate tag in 4.x? Then I would rather not to, because to be useful, the user will have to create a filter anyway (that changes the name of the new tag into PATH).

@ppatierno
Copy link
Member Author

I believe it's acceptable given this tag is disabled by default and there's a workaround for users who want the current behavior (which, again, is not consistent).

I would agree on just documenting it as a breaking change. In the end, it's a fix on a bug because the path reported right now is kind of wrong, so if users are using it as an URI they are using it wrongly imho.

@tsegismont
Copy link
Contributor

Hey @ppatierno

@vietj has approved the PR for master, I will soon merge it and backport it to 4.x. We'll just document the breaking change.

tsegismont added a commit that referenced this issue Oct 28, 2024
@ppatierno
Copy link
Member Author

@tsegismont that's a great news! Thank you very much for fixing this! When 4.5.11 is out I will let you know if our part will come back work again :-)

tsegismont added a commit to tsegismont/vertx-micrometer-metrics that referenced this issue Oct 29, 2024
tsegismont added a commit that referenced this issue Oct 29, 2024
@tsegismont
Copy link
Contributor

Fixed by #242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

4 participants