Fix ServletContextPath.prepend for app server spans #2089

laurit · 2021-01-21T14:29:05Z

ServletContextPath.prepend doesn't work when server span is created from app server integration because it uses a context that is only set up from servlet integration. Make app server integrations create this context and update it from servlet integration similarly to how span name is update. Modify struts tests that already declare a dependency on jetty integration to actually use jetty integration to verify that ServletContextPath.prepend actually prepends context path on jetty.

trask · 2021-01-27T07:23:33Z

...entation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java

+    Context context =
+        ctx.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(shouldRecordException));
+    // Add context for storing servlet context path. Servlet context path is updated from servlet
+    // integration.
+    if (context.get(ServletContextPath.CONTEXT_KEY) == null) {
+      context = context.with(ServletContextPath.CONTEXT_KEY, new AtomicReference<>(null));
+    }
+
+    return context;


instead of adding an AtomicReference so that we can set the contextPath into the Context without having to create a new scope, would it work to modify the Servlet instrumentation so that it creates a new Scope with the contextPath as needed in the case where normal Servlet span is suppressed due to upstream app server span?

I could add it in a separate advice

public static class ServletContextPathAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.Argument(0) ServletRequest request, @Advice.Local("otelScope") Scope scope) { if (!(request instanceof HttpServletRequest)) { return; } int callDepth = CallDepthThreadLocalMap.incrementCallDepth(ServletContextPath.class); if (callDepth > 0) { return; } HttpServletRequest httpServletRequest = (HttpServletRequest) request; String contextPath = httpServletRequest.getContextPath(); if (contextPath != null && !contextPath.isEmpty() && !contextPath.equals("/")) { Context context = Java8BytecodeBridge.currentContext().with(ServletContextPath.CONTEXT_KEY, contextPath); scope = context.makeCurrent(); } } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stop( @Advice.Local("otelScope") Scope scope) { if (scope != null) { CallDepthThreadLocalMap.reset(ServletContextPath.class); scope.close(); } } }

and apply it to same methods as Servlet3Advice/Servlet2Advice. But now the question is where should I place this advice class. Easiest would be to copy paste it to both servlet2 & servlet3 but I doubt that you'll accept that. If I add it to servlet-common with a separate instrumentation module then tests that have testInstrumentation project(":instrumentation:servlet:servlet-3.0:javaagent") wouldn't use it because servlet-3.0 doesn't depend on servlet-common. I could either make servlet-3.0 depend on servlet-common or add servlet-common to tests that use ServletContextPath.prepend but that would create another issue. Dispatcher and response instrumentation in servlet-common create extra spans that would need handling in tests. Advice appreciated.

I personally think that both servlet-2.2 and servlet-3.0 may depend on servlet-common. And in any case we should use all 2 (or even all 3) of them in tests. To be sure that they interplay correctly.

iNikem

Mother of dragons... Our tests are becoming something...

iNikem · 2021-02-01T15:07:16Z

@open-telemetry/java-instrumentation-approvers Anybody else wants to chime in?

trask · 2021-02-02T06:25:48Z

@laurit can you summarize the changes in this PR? there's a lot of new stuff since I first reviewed it that don't seem related to the original description above. thanks!

laurit · 2021-02-02T08:52:01Z

@trask sure.

move creation of servlet context path context to servlet-common and apply it on calls to Servlet.service and Filter.doFilter
add servlet-common dependency to tests that use servlet-api so that ServletContextPath.prepend would work in them
modify tests to account for extra spans created by RequestDispatcherInstrumentationModule and HttpServletResponseInstrumentationModule

trask · 2021-02-04T06:50:29Z

@laurit would it make sense to re-use the existing instrumentation points (Servlet2Advice/Servlet3Advice), instead of creating a new instrumentation point (ServletContextPathAdvice) that needs to be applied?

If we do keep the new instrumentation point, does it matter which order the instrumentation is run in (relative to Servlet2Advice/Servlet3Advice)? if so, we should probably use InstrumentationModule.getOrder() to make the ordering explicit

laurit · 2021-02-04T14:31:10Z

@trask i believe ordering is not important, neither advice depends on the other.
Doing it inside Servlet2Advice/Servlet3Advice would involve duplicating the advice code also then I would end up creating 2 scopes on entry that would both need to be closed on exit so would probably need to wrap the whole on exit method in try finally and close context path scope in finally. Or could try with 2 onenter and onexit method inside advice class, idk whether this would work at all.
My initial idea was to just have advice class in servlet-common and add it in ServletAndFilterInstrumentation but that backfired. After adding servlet-common dependency to servlet-3.0 muzzle assertInverse in jetty-8.0 fails because it assumes that all instrumentations fail, but the ones in servlet-common pass.

trask · 2021-02-04T22:33:43Z

Doing it inside Servlet2Advice/Servlet3Advice would involve duplicating the advice code also then I would end up creating 2 scopes on entry that would both need to be closed on exit so would probably need to wrap the whole on exit method in try finally and close context path scope in finally. Or could try with 2 onenter and onexit method inside advice class, idk whether this would work at all.
My initial idea was to just have advice class in servlet-common and add it in ServletAndFilterInstrumentation but that backfired. After adding servlet-common dependency to servlet-3.0 muzzle assertInverse in jetty-8.0 fails because it assumes that all instrumentations fail, but the ones in servlet-common pass.

Thanks, this makes sense.

I believe ordering is not important, neither advice depends on the other.

We do something surprising in the servlet advice, if it extracts the context from the request (e.g. no app server already extracts the context), then it uses Context.root(), so it won't inherit the contextPath if the ServletContextPathAdvice runs first.

opentelemetry-java-instrumentation/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java

Lines 193 to 195 in 121a9bf

    
           // Using Context.ROOT here may be quite unexpected, but the reason is simple. 
        
           // We want either span context extracted from the carrier or invalid one. 
        
           // We DO NOT want any span context potentially lingering in the current context.

trask · 2021-02-05T01:00:04Z

After adding servlet-common dependency to servlet-3.0 muzzle assertInverse in jetty-8.0 fails because it assumes that all instrumentations fail, but the ones in servlet-common pass.

Btw, just noticed @iNikem also ran into this muzzle issue when adding dependency on another javaagent instrumentation: #2156 (comment), he had solution there to extract library instrumentation for the shared code

laurit · 2021-02-05T16:19:16Z

@trask Thank, now context path is added after servlet integration.

trask

hey @laurit, sorry, thinking about this some more, from a performance perspective (since servlet is such a critical instrumentation), I think it's worth re-visiting if we can integrate this new instrumentation point into the existing Servlet2Advice/Servlet3Advice methods, so that we don't need to double the number of CallDepth thread local checks (and especially because these CallDepth checks need to be done inside of every doFilter, so there can be many in the filter chain).

If you would prefer to revisit as a separate PR, I'm good merging this as-is.

trask · 2021-02-06T06:12:42Z

I sent the beginning of a PR to your PR: laurit#2. Let me know if you see problems with that approach.

laurit · 2021-02-08T16:37:55Z

@trask that should work. With this adding servlet-common dependency and changes to tests shouldn't be needed any more, should I keep them or get rid of them?

trask · 2021-02-08T16:52:32Z

I think keep the servlet-common instrumentation addition to the tests. We want more realistic tests anyways.

…rom app server integration

…let2&3 depend on servlet-common so that it would be used in tests that depend on servlet3

iNikem · 2021-02-10T08:15:25Z

@trask If it is Ok with you, can you approve and merge?

laurit · 2021-02-10T08:53:10Z

@iNikem I'll do the changes that @trask suggested, should be done soon.

laurit · 2021-02-12T11:05:17Z

@trask could you review this again

trask

thanks @laurit

do you know why our smoke tests were passing prior to this PR? it looks like they are validating span names start with the context path /app

mostly wondering if we are missing some smoke test coverage

trask · 2021-02-13T03:00:37Z

...a/io/opentelemetry/javaagent/instrumentation/springwebmvc/HandlerAdapterInstrumentation.java

      if (serverSpan != null) {
        // Name the parent span based on the matching pattern
        tracer().onRequest(context, serverSpan, request);
-        // Now create a span for handler/controller execution.
-        span = tracer().startHandlerSpan(handler);
-        scope = context.with(span).makeCurrent();
      }
+      // Now create a span for handler/controller execution.
+      span = tracer().startHandlerSpan(handler);
+      scope = context.with(span).makeCurrent();
    }


is this change related to this PR? if not, can you send as a separate PR? since it's a behavior change, just want it to be more visible for others to review

Appserver tests only have a servlet, there the span name is set in servlet instrumentation and does not use ServletContextPath.prepend. SpringBootSmokeTest could possibly fail (din't really try), but it uses an empty context path so ServletContextPath.prepend doesn't do anything there anyway.

Probably should have mentioned this change as it should be the only non test change that was needed and there is a good chance that this is not the correct way to handle this case. This is needed for spring mvc SpringBootBasedTest "test spans with auth error" and a coupe of other spring mvc tests where the common theme is that these tests use an error page. This tests sets status to 401 Unauthorized after which servlet code exits and closes server span, after that tomcat forwards request to error page with request dispatcher. This error page is also implemented with spring so it hits servlet integration again where it is handled in Servlet3HttpServerTracer.needsRescoping(attachedContext) branch that restores the scope of the original request. Now when code reaches ControllerAdvice BaseTracer.getCurrentServerSpan(context); returns the already ended server span that was created in first servlet call and all is fine.
When servlet common instrumentation is enable then RequestDispatcherAdvice sets up a new scope, this makes servlet integration not take the Servlet3HttpServerTracer.needsRescoping(attachedContext) branch and eventually when the request reaches ControllerAdvice BaseTracer.getCurrentServerSpan(context); is null and creating handler span is skipped.

I reverted this changed and instead made springmvc tests run with tomcat instrumentation. With tomcat instrumentation RequestDispatcherAdvice does not cause loosing server span. Also fixed peer port and peer ip reporting in TomcatTracer. There is a slight side effect from using tomcat instrumentation, perviously showing error page did not update server span name as server span had already ended, now with tomcat instrumentation server span has not ended and error page updates server span name to /error.

trask · 2021-02-14T22:49:33Z

hey @laurit, I remembered a hacky workaround in JettyInstrumentationModule that we can remove now in this PR:

    @Override
    public ElementMatcher<TypeDescription> typeMatcher() {
      // skipping built-in handlers, so that for servlets there will be no span started by jetty.
      // this is so that the servlet instrumentation will capture contextPath and servletPath
      // normally, which the jetty instrumentation does not capture since jetty doesn't populate
      // contextPath and servletPath until right before calling the servlet
      // (another option is to instrument ServletHolder.handle() to capture those fields)
      return not(named("org.eclipse.jetty.server.handler.HandlerWrapper"))
          .and(not(named("org.eclipse.jetty.server.handler.ScopedHandler")))
          .and(not(named("org.eclipse.jetty.server.handler.ContextHandler")))
          .and(not(named("org.eclipse.jetty.servlet.ServletHandler")))
          .and(implementsInterface(named("org.eclipse.jetty.server.Handler")));
    }

…p reporting on tomcat

laurit · 2021-02-15T17:16:44Z

@trask removed jetty hack

This reverts commit 82cbb66.

This reverts commit c034496.

This reverts commit 8a3d077.

laurit · 2021-02-15T18:52:35Z

@trask removed jetty hack

Had to add it back. Turns out servlet-3.0 depends on jetty and removing it breaks async tests. When jetty integration applies to all handlers then jetty tests under servlet-3.0 would not be testing Servlet3Advice but rather jetty integration (would that be ok?). There will also be some extra spans. Might be better to address this in a separate pr.

trask

When jetty integration applies to all handlers then jetty tests under servlet-3.0 would not be testing Servlet3Advice but rather jetty integration (would that be ok?)

Ya, I think it will work out well, to have JettyServlet3Test test the AppServerBridge path, and have TomcatServlet3Test test the non-AppServerBridge path.

Might be better to address this in a separate pr.

Good idea 😅 👍

laurit requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners January 21, 2021 14:29

Base automatically changed from master to main January 26, 2021 05:50

trask reviewed Jan 27, 2021

View reviewed changes

iNikem approved these changes Jan 29, 2021

View reviewed changes

laurit force-pushed the servlet-context-path branch from 9a11e4a to dc2a241 Compare February 5, 2021 12:40

trask reviewed Feb 6, 2021

View reviewed changes

laurit added 7 commits February 10, 2021 00:56

ServletContextPath.prepend doesn't work when server span is created f…

a6a13d1

…rom app server integration

move ServletContextPath context creation to servlet-common, make serv…

8b4065c

…let2&3 depend on servlet-common so that it would be used in tests that depend on servlet3

fix failing test

57233d4

add servlet-common dependency to modules that depend on servlet3

50010b3

add servlet-common dependency to mojarra and myfaces

77d7d1d

run context path instrumentation after servlet instrumentation

3e39784

add servlet-common dependency to wicket

bb98885

move servlet context path handling

d03281e

laurit force-pushed the servlet-context-path branch from d2e12f8 to d03281e Compare February 10, 2021 09:57

trask reviewed Feb 13, 2021

View reviewed changes

laurit added 3 commits February 15, 2021 18:45

enable jetty instrumentation for all handlers

8a3d077

run springmvc tests with tomcat integration, fix peer port and peer i…

bc4acaa

…p reporting on tomcat

jetty integration is now enabled for all handlers

82cbb66

laurit added 4 commits February 15, 2021 19:23

update expected span name

c034496

Revert "jetty integration is now enabled for all handlers"

e952397

This reverts commit 82cbb66.

Revert "update expected span name"

eef7757

This reverts commit c034496.

Revert "enable jetty instrumentation for all handlers"

6083909

This reverts commit 8a3d077.

Trigger Build

0384452

trask approved these changes Feb 16, 2021

View reviewed changes

trask merged commit e76d3b1 into open-telemetry:main Feb 16, 2021

laurit deleted the servlet-context-path branch August 24, 2021 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ServletContextPath.prepend for app server spans #2089

Fix ServletContextPath.prepend for app server spans #2089

laurit commented Jan 21, 2021

trask Jan 27, 2021

laurit Jan 27, 2021

iNikem Jan 27, 2021

iNikem left a comment

iNikem commented Feb 1, 2021

trask commented Feb 2, 2021

laurit commented Feb 2, 2021

trask commented Feb 4, 2021

laurit commented Feb 4, 2021

trask commented Feb 4, 2021

trask commented Feb 5, 2021

laurit commented Feb 5, 2021

trask left a comment

trask commented Feb 6, 2021

laurit commented Feb 8, 2021

trask commented Feb 8, 2021

iNikem commented Feb 10, 2021

laurit commented Feb 10, 2021

laurit commented Feb 12, 2021

trask left a comment

trask Feb 13, 2021

laurit Feb 15, 2021

laurit Feb 15, 2021

trask commented Feb 14, 2021

laurit commented Feb 15, 2021

laurit commented Feb 15, 2021

trask left a comment

Fix ServletContextPath.prepend for app server spans #2089

Fix ServletContextPath.prepend for app server spans #2089

Conversation

laurit commented Jan 21, 2021

trask Jan 27, 2021

Choose a reason for hiding this comment

laurit Jan 27, 2021

Choose a reason for hiding this comment

iNikem Jan 27, 2021

Choose a reason for hiding this comment

iNikem left a comment

Choose a reason for hiding this comment

iNikem commented Feb 1, 2021

trask commented Feb 2, 2021

laurit commented Feb 2, 2021

trask commented Feb 4, 2021

laurit commented Feb 4, 2021

trask commented Feb 4, 2021

trask commented Feb 5, 2021

laurit commented Feb 5, 2021

trask left a comment

Choose a reason for hiding this comment

trask commented Feb 6, 2021

laurit commented Feb 8, 2021

trask commented Feb 8, 2021

iNikem commented Feb 10, 2021

laurit commented Feb 10, 2021

laurit commented Feb 12, 2021

trask left a comment

Choose a reason for hiding this comment

trask Feb 13, 2021

Choose a reason for hiding this comment

laurit Feb 15, 2021

Choose a reason for hiding this comment

laurit Feb 15, 2021

Choose a reason for hiding this comment

trask commented Feb 14, 2021

laurit commented Feb 15, 2021

laurit commented Feb 15, 2021

trask left a comment

Choose a reason for hiding this comment