-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(node): Pass inferred name & attributes to tracesSampler
#12945
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we can't get the final parameterized name in time, I think this is an improvement to what we had before so 👍
@@ -45,8 +45,6 @@ export function startSpan<T>(options: OpenTelemetrySpanContext, callback: (span: | |||
const spanOptions = getSpanOptions(options); | |||
|
|||
return tracer.startActiveSpan(name, spanOptions, ctx, span => { | |||
_applySentryAttributesToSpan(span, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this was actually also not ideal, we just added the op attribute after the span was started, which means this was not sent to the sampler. Also, this is overall unnecessary, we can just add the attribute right away!
Previously, we passed the span name directly to the
tracesSampler
as-is. However, this is not ideal because this does not match the name we actually send to sentry later, making this confusing. The reason is that we infer the name from the attributes for some types of spans, but we do that at export-time.This PR adjust this so that we send the inferred name & attributes to the
tracesSampler
. This works reasonably enough. However, there is a big caveat: This still only has access to the initial attributes that a span has at creation time. This means that we'll never have e.g. thehttp.route
correctly there, because allhttp.server
spans originate from the http instrumentation where they do not have a route yet, and then more specific instrumentation (e.g. express or fastify) add thehttp.route
to that span later. So the name will never be parametrized, for example, forhttp.server
spans, which is not ideal (as it will still not match the final span exactly) but should still be better than what we had so far (where the name would always just be e.g.GET
).Fixes #12944