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

Late bind of the Span name in SpanNameExtractor #4716

Closed
radcortez opened this issue Nov 25, 2021 · 9 comments
Closed

Late bind of the Span name in SpanNameExtractor #4716

radcortez opened this issue Nov 25, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@radcortez
Copy link
Contributor

Is your feature request related to a problem? Please describe.
As described in #442 (comment), the Span provides a updateName method for cases when the span name is not known on the span start. In the Instrumenter API, the span name is set via a SpanNameExtractor at the start, and then the only way to update it is to manually retrieve the Span to update the name.

Describe the solution you'd like
A possible solution may be to add an additional default method to SpanNameExtractor that allows you to implement a late bind to the span name that calls updateName in Span.

Describe alternatives you've considered
Alternatively, in the Instrumenter implementation on end, a second call to SpanNameExtractor#extract could be made to update the name, but it may not be completely clear to the consumer that this is happening and could cause some undesirable side effects.

I'm available to submit a PR for this.

@radcortez radcortez added the enhancement New feature or request label Nov 25, 2021
@trask
Copy link
Member

trask commented Nov 28, 2021

hey @radcortez!

Describe the solution you'd like
A possible solution may be to add an additional default method to SpanNameExtractor that allows you to implement a late bind to the span name that calls updateName in Span.

When would this method get called (or who would call it)?

Describe alternatives you've considered
Alternatively, in the Instrumenter implementation on end, a second call to SpanNameExtractor#extract could be made to update the name, but it may not be completely clear to the consumer that this is happening and could cause some undesirable side effects.

I'd personally like to keep span name updates at early as possible. In our Java agent distro, we treat the "local root" span name as the "operation name" and copy that down to the child spans, since things like "show me sql queries for /login` is a common ask, and copying the local root span name down to the child spans avoids backend indexing / joining.

@radcortez
Copy link
Contributor Author

hey @radcortez!

Describe the solution you'd like
A possible solution may be to add an additional default method to SpanNameExtractor that allows you to implement a late bind to the span name that calls updateName in Span.

When would this method get called (or who would call it)?

The instrumenter can call it internally, but with the current API, it can only happen when you call end since you only have start and end available, and if you don't have the span name available during start, the only option is on end.

Describe alternatives you've considered
Alternatively, in the Instrumenter implementation on end, a second call to SpanNameExtractor#extract could be made to update the name, but it may not be completely clear to the consumer that this is happening and could cause some undesirable side effects.

I'd personally like to keep span name updates at early as possible. In our Java agent distro, we treat the "local root" span name as the "operation name" and copy that down to the child spans, since things like "show me sql queries for /login` is a common ask, and copying the local root span name down to the child spans avoids backend indexing / joining.

To be more precise and give more detail, I've implemented a Vert.x integration using the Instrumenter API. The issue here is that on Vert.x I'm only able to discover the route name very late in the request handling (which contributes to the span name). I may be able to delay the start of the instrumenter, but there is a chance to miss other telemetry data.

Right now, I've worked around this with a custom AttributesExtractor and on onEnd method, getting the current Span and then calling updateName with my route name.

@mateuszrzeszutek
Copy link
Member

In case of http.route there's ServerSpanNaming that can update the SERVER span name with the actual route discovered later. Unfortunately it doesn't set the span attribute (yet; but I think it could and should be refactored to do that), but I believe that this pattern will have to stay with us in this repo -- most HTTP server libraries expose route information late (or not at all), so we have to work around the start-end instrumenter limitations.

@radcortez
Copy link
Contributor Author

most HTTP server libraries expose route information late (or not at all), so we have to work around the start-end instrumenter limitations.

Exactly. My ask was to just provide a more standard way to update it.

@trask
Copy link
Member

trask commented Nov 29, 2021

Right now, I've worked around this with a custom AttributesExtractor and on onEnd method, getting the current Span and then calling updateName with my route name.

I think I understand, so in your case, you are able to extract the route directly from the <REQUEST, RESPONSE> that are already provided by the instrumenter?

Most of the existing instrumentation requires additional instrumentation (advice) to capture the route. So this "route capturing" instrumentation calls Span.updateName() directly (via ServerSpanNaming). Though this needs to be improved in order to capture and pass http.route in a way that it can be stamped onto metrics: #4556 (comment). Would something like this work for your use case also?

@radcortez
Copy link
Contributor Author

Right now, I've worked around this with a custom AttributesExtractor and on onEnd method, getting the current Span and then calling updateName with my route name.

I think I understand, so in your case, you are able to extract the route directly from the <REQUEST, RESPONSE> that are already provided by the instrumenter?

Yes. Is just that I don't have it on instrumenter.start(), but I do have it in instrumenter.end().

Most of the existing instrumentation requires additional instrumentation (advice) to capture the route. So this "route capturing" instrumentation calls Span.updateName() directly (via ServerSpanNaming). Though this needs to be improved in order to capture and pass http.route in a way that it can be stamped onto metrics: #4556 (comment). Would something like this work for your use case also?

Yes, this could work. In essence, this is what I'm doing right now. I pass in the context in a wrapped object in <REQUEST,RESPONSE> and then I'm able to update the span name and set the route.

@mateuszrzeszutek
Copy link
Member

Hey @radcortez ,
You should now be able to use the HttpRouteHolder class to update the server span name and set the http.route attribute after instrumenter.start(). Please take a look at it -- does this solve your use case?

@radcortez
Copy link
Contributor Author

Thanks. Let me have a look.

@radcortez
Copy link
Contributor Author

Hey @radcortez ,
You should now be able to use the HttpRouteHolder class to update the server span name and set the http.route attribute after instrumenter.start(). Please take a look at it -- does this solve your use case?

I confirm it solves the use case. Thank you!

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

No branches or pull requests

3 participants