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.

Sign up for GitHub

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

Jump to bottom

Add context root to the front of the route in the span name #1418

Merged
merged 20 commits into from
Oct 27, 2020
Merged

Add context root to the front of the route in the span name #1418

merged 20 commits into from
Oct 27, 2020

Conversation

trask
Copy link
Member

@trask trask commented Oct 19, 2020 •
edited

Resolves #1299

This PR does a few things, please see commit breakout:

  • Add application root to span name
    • This commit keeps track of the context path, and prepends it to the route, e.g. old span name was '/mycontroller', new span name is '/mycontext/mycontroller'. If there is no context path (app is deployed to root) then there is no change
  • Reduce duplicate context lookups
    • Since the prior commit added extra context lookups, this commit consolidated them where there was already a context lookup being done
  • User servlet path as route
    • Previously, we had span names like ApplicationFilterChain.doFilter and HttpServlet.service. Now it uses request.getServletPath() for the span name instead.
  • Test against jetty instrumentation
    • The jetty instrumentation occurs before getContextPath() and getServletPath() are available, so a few changes were needed to handle this better (see comments in this commit for alternative options)

Currently missing tests for non-root context path in

  • jaxrs-1.0
  • webflux

@trask trask force-pushed the add-application-root-to-span-name branch from 8d99287 to 319453f Compare October 22, 2020 22:29
@trask trask marked this pull request as ready for review October 23, 2020 03:43
@trask
Copy link
Member Author

trask commented Oct 24, 2020 •
edited

this is ready for review EDIT: back to draft..

@trask trask mentioned this pull request Oct 25, 2020
Closed
@trask trask force-pushed the add-application-root-to-span-name branch from 9ac0afc to a153af5 Compare October 26, 2020 04:17
@trask trask mentioned this pull request Oct 26, 2020
Merged
@trask trask force-pushed the add-application-root-to-span-name branch from a153af5 to 8db6d34 Compare October 26, 2020 04:49
@trask
Copy link
Member Author

trask commented Oct 26, 2020

See the updated the description for explanation of the different things in this PR.

I can submit the commits one-by-one if that's preferred.

anuraaga
anuraaga reviewed Oct 26, 2020
View reviewed changes
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java Outdated Show resolved Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxRsAnnotationsTracer.java Show resolved Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JaxRsAnnotationsTracer.java Outdated Show resolved Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/spring/webflux/server/HandlerAdapterAdvice.java Show resolved Hide resolved
...rc/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java Outdated Show resolved Hide resolved
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java Outdated Show resolved Hide resolved
iNikem
iNikem suggested changes Oct 26, 2020
View reviewed changes
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java Outdated Show resolved Hide resolved
...vlet-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java Show resolved Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxRsAnnotationsTracer.java Outdated Show resolved Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxRsAnnotationsTracer.java Outdated Show resolved Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxRsAnnotationsTracer.java Show resolved Hide resolved
...rc/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/SpringWebMvcTracer.java Outdated Show resolved Hide resolved
@trask
Copy link
Member Author

trask commented Oct 27, 2020

Added note to description:

Currently missing tests for non-root context path in

  • jaxrs-1.0
  • webflux

The tests may take some work, so would prefer to create separate issue to track.

@trask
Copy link
Member Author

trask commented Oct 27, 2020

I'm not sure we can always get the servlet request statically (e.g. jersey).

If we can't get the servlet context path everywhere, then I think probably better to use the ServletContextPath context key everywhere for consistency (since at that point we will need to propagate it anyways). The latest commit re-consolidates to this (now better formed) pattern.

Let me know your thoughts.

anuraaga
anuraaga approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Had no idea I was using jetty, not jersey. Servlet's very complicated 😅

...ation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServletContextPath.java
import io.opentelemetry.context.Context;
import io.opentelemetry.context.ContextKey;

public class ServletContextPath {
Copy link
Contributor

Choose a reason for hiding this comment

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

ServletContextContextUtils? Just kidding :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some class javadoc to explain what's going on, including why this needs to go in instrumentation-api

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will do tomorrow before merging

@trask trask mentioned this pull request Oct 27, 2020
Open
@trask trask merged commit f046e59 into open-telemetry:master Oct 27, 2020
@trask trask deleted the add-application-root-to-span-name branch October 27, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuraaga anuraaga anuraaga approved these changes

@iNikem iNikem iNikem approved these changes

@jkwatson jkwatson Awaiting requested review from jkwatson

@mateuszrzeszutek mateuszrzeszutek Awaiting requested review from mateuszrzeszutek

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add context root to the front of the route in the span name
3 participants
@trask @anuraaga @iNikem