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 JDK http client propagation of non-sampled traces #3736

Merged
merged 3 commits into from
Aug 2, 2021
Merged

Fix JDK http client propagation of non-sampled traces #3736

merged 3 commits into from
Aug 2, 2021

Conversation

trask
Copy link
Member

@trask trask commented Jul 30, 2021

And adds missing test coverage to HttpClientTest

// sleep to ensure no spans are emitted
Thread.sleep(1000);

assertThat(testing.traces()).isEmpty();
Copy link
Member

Choose a reason for hiding this comment

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

How does this verify that the proper headers are being sent? Would you expect the server to generate a trace if it wasn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

ya 👍

if (Java8BytecodeBridge.currentSpan().isRecording()) {
headers = tracer().inject(headers);
}
headers = tracer().inject(headers);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like I just happened to pick an unlucky client. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

or a lucky one, to find this bug 💯

@trask
Copy link
Member Author

trask commented Jul 30, 2021

closing and re-opening to re-trigger CLA check

@trask trask closed this Jul 30, 2021
@trask trask reopened this Jul 30, 2021
@caniszczyk
Copy link

/easycla

assertThat(responseCode).isEqualTo(200);

// sleep to ensure no spans are emitted
Thread.sleep(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

1s seems too slow how about around 200? I think we get traces back in just a few ms most times.

@anuraaga anuraaga merged commit 03632d9 into open-telemetry:main Aug 2, 2021
@trask trask deleted the fix-jdk-http-client-propagation branch August 3, 2021 03:21
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