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

otelhttp should collect url information #3743

Open
bjornbyte opened this issue Apr 26, 2023 · 10 comments
Open

otelhttp should collect url information #3743

bjornbyte opened this issue Apr 26, 2023 · 10 comments
Labels

Comments

@bjornbyte
Copy link

Problem Statement

When I instrument my application using otelhttp.NewHandler(...) the spans it produces are missing method and path information which is important for understanding my system. The spans for requests it sends to other services (as a client) include this information, but the spans for requests it receives do not.

Proposed Solution

Either by default, or by some WithUrlAttributes option, it spans for incoming http requests should include Url information and set the standard http.url attributes.

@bjornbyte bjornbyte added area: instrgen Related to the instrgen package enhancement New feature or request labels Apr 26, 2023
@dmathieu
Copy link
Member

Could you provide a sample of the issue?
The Handler sets Server attributes from semconv, which does add the http.method.

As for http.target, it was removed a few months ago due to cardinality issues (which seems weird for traces, I agree): open-telemetry/opentelemetry-go#3687

I'll let @MrAlias chime in, but I think the intent was to remove it for metrics (where high cardinality is indeed an issue). So we could maybe add it only for traces?

@bjornbyte
Copy link
Author

My mistake, yes, it does include method.
image

What I'm really missing is the url path in the incoming request, but could also get that from http.url like it has on outgoing (client) requests.

My current workaround is to add an additional filter in my request pipeline and add the attribute,

trace.SpanFromContext(req.Request.Context()).SetAttributes(
		attribute.String("http.url.path", req.Request.URL.Path),
	)

which just now looking at https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md I realize should be http.target or http.route, but anyway, it would be really nice to have an easy way to have that added for me already when using the otelhttp handler.

@dmathieu
Copy link
Member

net/http has no notion of route, so it should be http.target, which the spec does mention as a required field.
I do agree this attribute should be there for traces (so does the spec).

@dashpole
Copy link
Contributor

dashpole commented Oct 6, 2023

I ran into this, and came up with a workaround:

I changed this:

otelhttp.NewHandler(handler, serviceName, opts...)

To this:

wrappedHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
  // Add the http.target attribute to the otelhttp span
  if r.URL != nil {
	  oteltrace.SpanFromContext(r.Context()).SetAttributes(semconv.HTTPTarget(r.URL.RequestURI()))
  }
  handler.ServeHTTP(w, r)
})
otelhttp.NewHandler(wrappedHandler, serviceName, opts...)

@alexchowle
Copy link

net/http has no notion of route, so it should be http.target, which the spec does mention as a required field. I do agree this attribute should be there for traces (so does the spec).

Looking at the latest specification, the HTTP Server Span should definitely provide url.path. I can't find a reference to http.target as a required field.

@tonglil
Copy link
Contributor

tonglil commented Apr 10, 2024

@dennis-wielepsky
Copy link

We would love to use x-ray based sampling in respect of different routes on a reverse proxy (caddy). Sadly the missing propagation of http.targetUrl leads to the point where this is not possible.

Even the great workaround from @dashpole did not work here, as those context is added after the sampling decision.

Does anybody have an idea how to add the targetUrl so the sampler will respect that?

@dashpole
Copy link
Contributor

dashpole commented May 8, 2024

Migrating to the new semconv is tracked in https://github.com/orgs/open-telemetry/projects/87. We are required to implement the migration plan specified here: https://github.com/open-telemetry/semantic-conventions/tree/main/docs/http#semantic-conventions-for-http.

@dennis-wielepsky
Copy link

dennis-wielepsky commented May 8, 2024

Thanks for your quick reply. Sadly I have no access to the project (seems to be private).

As we are not as experienced in OTEL / go instrumentation we found a hacky solution by providing a proxied TraceProvider which will read the http.url from a specific value provided within the request context before calling the Tracer.

That workaround should be sufficient to enable the request based sampling 😄

@dashpole
Copy link
Contributor

dashpole commented May 8, 2024

Ah, you can follow along here: #5331

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

No branches or pull requests

7 participants