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

PAYARA-2924 NPE on JAX-RS Async-Server EE7 Sample #3041

Merged

Conversation

Pandrex247
Copy link
Member

If request tracing is turned on when running this sample, a NPE is thrown.
This is caused by the return happening on a separate thread, so the container filter gets a NPE when trying to get the access active span (as these are thread local).

This PR fixes this by saving the span context as an attribute of the InvocationContext object created by managed pools so that the span context can be propagated when setting up the thread context.

Threads from managed pools created external to Payara will not be traced however, such as those created by Jersey to handle timeouts.

This also adds the ability for OpenTracing to create traces itself (rather than requiring a trace to already be running), and includes a couple bits of cleanup.

@mulderbaba mulderbaba added this to the Payara 5.183 milestone Aug 15, 2018
@lprimak
Copy link
Contributor

lprimak commented Aug 15, 2018

Have you tested with versioned applications? I think you will get a clash.
This is why my first stab at that "context"-y kind of stuff was by application name, but I went off that and started using component ID instead.
I think you are re-inventing the wheel here with maps and stuff and better off using JavaEEContextUtil

@Pandrex247
Copy link
Member Author

I went with using this method as it's essentially how they suggest you do it in OpenTracing.

I'm not sure where I'd be getting a clash though, as the application name is only ever used for getting the Tracer instance, which is thread local anyway.
And if there was a clash, it's not strictly tied to this fix.
I may just be being blind though, so I'll give it a test.

@Pandrex247
Copy link
Member Author

Jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@arjantijms arjantijms merged commit 7ab5253 into payara:master Aug 24, 2018
@arjantijms
Copy link
Contributor

Have you tested with versioned applications?

Not yet, might look at that next version.

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.

5 participants