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

otelmux: Add config option for a custom span name formatter #3040

Closed
hairyhenderson opened this issue Nov 28, 2022 · 4 comments · Fixed by #3041
Closed

otelmux: Add config option for a custom span name formatter #3040

hairyhenderson opened this issue Nov 28, 2022 · 4 comments · Fixed by #3041

Comments

@hairyhenderson
Copy link
Contributor

Currently, spans created in the otelmux instrumentation module are named after the route template (or the route regexp if no template is available). For example, given a route of /users/{id}, spans will be named /users/{id}.

This is fine in general, but it's impossible to differentiate between handlers for different methods. For example, if there are GET /users/{id} and DELETE /users/{id} handlers, the only way to differentiate spans is to visually inspect the http.method attribute.

In the otelhttp module, a WithSpanNameFormatter option function is available, which allows users to generate custom names on a per-request basis.

I propose adding a similar WithSpanNameFormatter option function to the otelmux module, with the difference that where otelhttp's function provides the handler's operation name, the otelmux version would provide the route name (to avoid needing to re-compute it on every request).

I have a PR already in progress that I will link to this issue.

@dmathieu
Copy link
Member

The specification recommends using VERB ROUTE as the default span name for HTTP instrumentations.

So, while a WithSpanNameFormatter option sounds good, I think the default should be changed as well.

@hairyhenderson
Copy link
Contributor Author

I'm open to changing the default (in fact, that would be ideal in my particular usage), but I can't find any reference to that form in the spec at the link you provided. I do however see:

Given an inbound request for a route (e.g. "/users/:userID?") the name attribute of the span SHOULD be set to this route. If the route does not include the application root, it SHOULD be prepended to the span name.

(from the HTTP Server semantic conventions section)

@dmathieu
Copy link
Member

It's more a hint than anything explicit. But being consistent with net/http would be better IMHO.

Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}".

@hairyhenderson
Copy link
Contributor Author

I guess we're reading that sentence in the spec differently... That one in particular is about client spans (not server spans, which is what this issue is about), and it's explicitly talking about a low-cardinality name like "HTTP {METHOD}" (not "HTTP {METHOD} {PATH}" which would be higher-cardinality...

IMO the spec isn't clear enough, and should needs to be updated.

However, my goal here is not change the default, but instead to simply provide some more configurability. If the default can change later in a separate issue/PR/spec change, that's totally fine, but I'd much rather move this specific piece forward first.

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

Successfully merging a pull request may close this issue.

2 participants