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

using active span if exists instead of extracting from http headers #74

Merged

Conversation

tmszdmsk
Copy link
Contributor

@tmszdmsk tmszdmsk commented Feb 2, 2018

Proof of concept for #67.

If there is active span already (probably initialized in a different filter, e.g. servlet filter), we should use it instead of extracting a new one from HTTP headers.

How do you think would be the best way to test that?

…ts instead of extracting from http headers
Copy link
Collaborator

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

@tmszdmsk thanks! It looks good.

Tests are based on Jetty, so we can use jetty filter to test it. You will have to create a new class which extends AbstractJetty test and then subclass it in every jax-rs provider. Or maybe there is a better way :)

Format.Builtin.HTTP_HEADERS,
new ServerHeadersExtractTextMap(requestContext.getHeaders())
);
if (extractedSpanContext != null) {
Copy link
Collaborator

@pavolloffay pavolloffay Feb 5, 2018

Choose a reason for hiding this comment

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

nit: This if looks redundant.

This method could be a lot simpler without SESE. Just return if active is not null. The default return can be direct return tracer.extract()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, it's because I extracted this method and didn't review it later ;)

@tmszdmsk
Copy link
Contributor Author

tmszdmsk commented Feb 5, 2018

Ok, I will work on adding tests for that.

@tmszdmsk
Copy link
Contributor Author

tmszdmsk commented Feb 6, 2018

FYI: it's "in progress". I've added the first test checking this functionality.

@pavolloffay
Copy link
Collaborator

It looks good, could you please add the test to other jax-rs providers?

@tmszdmsk
Copy link
Contributor Author

tmszdmsk commented Feb 6, 2018

@pavolloffay Sure, I will work on it probably today.

@pavolloffay pavolloffay merged commit 1ead39f into opentracing-contrib:master Feb 6, 2018
@tmszdmsk tmszdmsk deleted the use-active-span-when-exists branch February 6, 2018 16:22
@tmszdmsk tmszdmsk restored the use-active-span-when-exists branch February 6, 2018 17:06
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.

2 participants