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

Clarify url.template value in case of redirects #1143

Open
lmolkova opened this issue Jun 11, 2024 · 3 comments
Open

Clarify url.template value in case of redirects #1143

lmolkova opened this issue Jun 11, 2024 · 3 comments

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Jun 11, 2024

Extracted from #1095

In case user provides a template similarly to

 Mono<Account> result = client.get()
         .uri("/users/{id}", 43)
         .exchange()
         .then(response -> response.bodyToMono(Account.class));

or

// imaginary
using (Context.AddAttribute("url.template", "/users/{id}")) {
    await httpClient.SendAsync(request);
}

but then a redirect happens, then it's not clear whether url.template should apply to all HTTP spans created within this scope.

E.g. we can end up with:
Span 1:

name: `GET users/{id}`
url.template: https://host:port/users/{id}
url.full: https://host:port/users/123
http.response.status_code = 302

Span 2:

name: `GET users/{id}`                      <- the same as on span 1 and not a template of `url.full` on this span    
url.template: https://host:port/users/{id}  <- the same as on span 1 and not a template of `url.full` on this span
url.full: https://anotherhost/something/else/42?foo=bar
http.response.status_code = 200

In case of Java example with Spring WebClient above, it's clear that template applies only to the first request.
In the case of the second example (ambient context) it's not clear at all.

We can probably tolerate it (possible mismatch in template/url for redirects), but we should provide some clarity in the spec (e.g. something along the "the template SHOULD be related to the url on the same span" lines).

@lmolkova
Copy link
Contributor Author

/cc @trask

@trask
Copy link
Member

trask commented Jun 11, 2024

I think url.template should be related to url.full on the same span

maybe we can make this explicit by changing:

The low-cardinality template of an absolute path reference.

to

The low-cardinality template of the absolute path reference portion of url.full.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 11, 2024

agreed.
Still, the ability to follow this would depend on the mechanism used to provide url.template.
It won't be possible with context-scoped attributes:

// imaginary
using (Context.AddAttribute("url.template", "/users/{id}")) {
    await httpClient.SendAsync(request);
}

Even without redirects, everything that's a pure user input is should and not SHOULD.

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

No branches or pull requests

3 participants