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

[ppatierno]

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 https://github.com/strimzi/strimzi-kafka-bridge/issues/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]

@jotak I see you made the change with this commit https://github.com/vert-x3/vertx-micrometer-metrics/commit/f19fe628357526f3e518bf16d8a3dded33d78222 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]

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]

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 https://github.com/eclipse-vertx/vert.x/commit/c1eb9997d5041f3b6ebbf2a34d1e9de16948a758 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]

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 https://github.com/eclipse-vertx/vert.x/commit/c1eb9997d5041f3b6ebbf2a34d1e9de16948a758 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]

@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]

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]

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

[ppatierno]

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]

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

[ppatierno]

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]

@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]

@ppatierno I'ill have a loop asap

[vietj]

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

[ppatierno]

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]

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

[vietj]

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

[ppatierno]

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

[tsegismont]

@vietj @ppatierno I've sent https://github.com/vert-x3/vertx-micrometer-metrics/pull/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]

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

[tsegismont]

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]

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]

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.

[ppatierno]

@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]

Fixed by #242

[tsegismont]

Documentation: https://github.com/vert-x3/wiki/wiki/4.5.11-Deprecations-and-breaking-changes#http-path-metrics-dont-consistently-report-a-path