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

fix order of cxf handlers to enable symmetric tracing around jaxws handler chain #8160

Conversation

pellmont
Copy link
Contributor

the current implementation of Start and End around the invocation of a Jax WS is asymmetric around the JAX-WS Handler Chain.

Current behavior:
(execution of incoming MessageHandlers) -> (TracingStartInInterceptor) -> (WebService Invocation) -> (execution of outgoing MessageHandlers) -> (TracingEndInInterceptor)

if I understood the code of this cxf instrumentation correctly, the intent was to build the span close around the WebService Invocation (without Handler Chains).

So the desired behavior would look like this:
(execution of incoming MessageHandlers) -> (TracingStartInInterceptor) -> (WebService Invocation) -> (TracingEndInInterceptor) -> (execution of outgoing MessageHandlers)

Unfortunately CXF is calling the Outgoing Chain inside the POST_INVOKE Phase of Cxf (so the outgoing chain is technically a sub-chain in the incoming chain... which is documented but quite surprising...).

So the solution in the fix at least guarantees the the outgoing chain is invoked AFTER end of tracing. For any extra Interceptors in the POST_INVOKE Phase there is still no guarantee of ordering, but I think this is not a opentelemetry issue but a design-flaw of CXF...

@pellmont pellmont requested a review from a team as a code owner March 29, 2023 08:41
@laurit
Copy link
Contributor

laurit commented Mar 29, 2023

I think we should also modify tests. Perhaps add

    endpoint.getOutInterceptors().add(new AbstractPhaseInterceptor<Message>(Phase.SEND) {
      @Override
      void handleMessage(Message message) throws Fault {
        Context context = Context.current()
        if (LocalRootSpan.fromContext(context) != Span.fromContext(context)) {
          throw new IllegalStateException("handler span should not be ended before outgoing interceptors")
        }
      }
    })

after

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thanks @pellmont!

can you also update the test as @laurit suggested?

…a/io/opentelemetry/javaagent/instrumentation/cxf/TracingEndInInterceptor.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@pellmont pellmont force-pushed the bugfix/symmetric-tracing-between-jaxws-handler-chain branch from d029b2d to 29b94bc Compare April 1, 2023 06:40
@pellmont pellmont force-pushed the bugfix/symmetric-tracing-between-jaxws-handler-chain branch from 29b94bc to 5a6f44f Compare April 1, 2023 07:14
@laurit laurit merged commit 5db149e into open-telemetry:main Apr 3, 2023
@pellmont
Copy link
Contributor Author

pellmont commented Apr 3, 2023

Thanks @trask and @laurit !

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.

None yet

3 participants