Allow retrieval of the http path including the "/" prefix

[codefromthecrypt]

It would be great to include the full HTTP path, not the path except the leading slashes, as that's the data used in trace tags, parsing and sampling. spring-cloud/spring-cloud-sleuth#1554

Motivation

We are trying to reduce the overhead of reactor instrumentation in sleuth. While most of this is propagation related, the most commonly requested tag, "http.path" is the entire path, not the path except the leading and trailing slash. When reactor strips these, it causes us to add them back, allocating new arrays to fix it, increasing overhead vs other HTTP libraries.

Desired solution

Either the .path() returns the entire path, no stripping, or something else is provided that does without excess allocations implied.

Considered alternatives

We will have to allocate strings meanwhile

Additional context

it is unusual for the path to strip the leading slash, as this will cause garbage that's likely unnecessary here. The only library I'm aware of that does this also is jersey, though that's the server.. I'm not sure what their client does.

[violetagg]

@adriancole Which one you need in your component

uri (HttpClientRequest#uri()) - which includes path, query, fragments
or
path (HttpClientRequest#path()) - which is just the path fragment from the uri

[codefromthecrypt]

we need both, we have hooks for both, but the path is collected by default. There are many users who use the hook to get the full url (not just uri, actually fully constructed URL including query), just we don't collect that by default for security reasons.

[codefromthecrypt]

in some cases, ex in armeria/netty, the URL can be tricky to construct because of the problem of who knows if it is https or not, and who knows the hostname. This is understood.. and I recall them having to change internals to have that work by default.

Anyway, the most important thing is the path, but yeah also getting the whole URL would be great.

[codefromthecrypt]

in fact, one of the remaining test failures in sleuth is supportsPortableCustomization() which is failing only because we have to add code to reconstruct the whole URL given the uri

[violetagg]

@adriancole you have the URL
Use HttpClientRequest#resourceURL()

[codefromthecrypt]

@violetagg cool thanks, that works. sorry about the distractions.. we can focus this on the /path thing!

[violetagg]

@rstoyanchev If we want to address this it will be an incompatible change. Wdyt?

[codefromthecrypt]

one thing that possibly could be of interest is if this stripping was done for micrometer. I don't know this.. just guessing why we would remove slashes (as it is arguable that /foo/ is a different endpoint than /foo)

If this was originally the case, we should check that we aren't redoing them anyway in micrometer, because I think it has a tag normalizer that does things besides this (Ex trimming duplicate slashes internally iirc).

[violetagg]

@adriancole no it is not for micrometer, it is like this since ever https://github.com/reactor/reactor-netty/blob/0.6.x/src/main/java/reactor/ipc/netty/http/HttpInfos.java#L62

[rstoyanchev]

I do agree that stripping the leading and trailing slash is unusual. However, given that it's existing behavior, and based on the way it's documented I see no room for changing that. I can only imagine an alternative method, something like fullPath.

[violetagg]

@adriancole @rstoyanchev PTAL #1025