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

Refactor ServerSpanNaming (in preparation for http.route) #4852

Conversation

mateuszrzeszutek
Copy link
Member

First part of #442

I've refactored ServerSpanNaming (mostly in the servlet instrumentation) so that it's called every time we include the route in the span name. In next PRs I want to remove/deprecate the HttpServerAttributesExtractor#route() method and only use ServerSpanNaming (which will be renamed to HttpServerRoute probably) to set the route as span name & attribute.

@laurit
Copy link
Contributor

laurit commented Dec 9, 2021

Have you considered that what we use as server span name is not always a route. For example wicket instrumentation sets server span name to an arbitrary value whose only purpose is to identify the visited page.

@mateuszrzeszutek
Copy link
Member Author

For example wicket instrumentation sets server span name to an arbitrary value whose only purpose is to identify the visited page.

Oh, didn't know that. Most of the web libraries that we instrument use route to update the name I think.
How many of these "special" instrumentations that do not update the span name with route do we have?

@laurit
Copy link
Contributor

laurit commented Dec 9, 2021

Oh, didn't know that. Most of the web libraries that we instrument use route to update the name I think. How many of these "special" instrumentations that do not update the span name with route do we have?

A few: grails, jaxws, spring-ws, tapestry, vaadin, wicket. For non-rest frameworks we need to invent some kind of name that describes the operation. This name isn't necessarily even deduced from url. For example in case of jax-ws it is a soap xml document that is posted that determines what is invoked.

@mateuszrzeszutek
Copy link
Member Author

A few: grails, jaxws, spring-ws, tapestry, vaadin, wicket.

Thanks, I'll take a look at these.
The spec definition of http.route is pretty loose (The matched route (path template)), so I wonder if it's fine to leave at least some of these as they are (template file names?).

Anyway, this PR is still valid even with these reservations -- in the worst case I'll just have to think of how to split the server name from the route so that the code still makes any sense 😅

context = ServerSpanNaming.init(context, CONTAINER);
return new AppServerBridge.Builder().init(context);
})
(context, request, attributes) -> new AppServerBridge.Builder().init(context))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should ServerSpanNaming.get() context customizer be added here? I notice it's missing in some cases and included in others, but not clear to me why

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually is added, via the ServletInstrumenterBuilder class.
All server instrumentations should add ServerSpanNaming.get()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, indeed!

context = ServerSpanNaming.init(context, CONTAINER);
return new AppServerBridge.Builder().init(context);
})
(context, request, attributes) -> new AppServerBridge.Builder().init(context))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, indeed!

Comment on lines +73 to +74
contextToUpdate =
helper().updateContext(contextToUpdate, httpServletRequest, mappingResolver, servlet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous behavior was nice of setting everything up correctly in startSpan (e.g. correct span name and correct ServerSpanNaming.Source)

though I guess it doesn't matter in practice most of the time since we instrument so many servlet containers that we probably don't startSpan that often here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous behavior was nice of setting everything up correctly in startSpan (e.g. correct span name and correct ServerSpanNaming.Source)

Honestly I found setting the Source a bit confusing: what if your span name was HTTP GET with FILTER as a source? If you had another filter trying to update the name to /abc it wouldn't, because it was a shorter name - which I'm not so sure is correct; setting the source seems to be closely connected to the route, not necessarily the span name (even if they're the same in most cases). (Although I'm not sure how "real" this example is 😅 )

Also, the only 2 instrumentations I found that have access to the route in the beginning are armeria and servlets; and servlet span name will usually be updated by something like spring or jax-rs. Which implies that for most cases, the route should be extracted on end() (and span name updated somewhere in the meantime).

@mateuszrzeszutek mateuszrzeszutek merged commit a65e963 into open-telemetry:main Dec 14, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the refactor-ServerSpanNaming branch December 14, 2021 09:15
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
…elemetry#4852)

* Refactor ServerSpanNaming (in preparation for http.route)

* fix tests

* Add ServerSpanNaming to all HTTP server instrumentations

* fix tests
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 this pull request may close these issues.

3 participants