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

Prevent HttpPropagator errors given a nil Context #502

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Jul 30, 2018

If you provide HttpPropagator#inject! a nil Context, it will raise an error. This pull request adds a simple check to avoid errors if traces can't be propagated for this reason.

Depends on #501 which refactors some tests.

@delner delner added bug Involves a bug integrations Involves tracing integrations labels Jul 30, 2018
@delner delner self-assigned this Jul 30, 2018
@delner delner requested a review from palazzem July 30, 2018 18:30
@delner delner force-pushed the bugfix/prevent_http_propagation_nil_context branch from 01c1556 to 370ece8 Compare July 30, 2018 19:03
palazzem
palazzem previously approved these changes Jul 30, 2018
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Even if our assumption is that a Span has always a Context, there is still a code-path that doesn't honor that. Let's add this safe-guard in any case, making the code more robust.

@delner delner force-pushed the bugfix/prevent_http_propagation_nil_context branch from 370ece8 to 1dce53a Compare July 30, 2018 22:03
@delner
Copy link
Contributor Author

delner commented Jul 30, 2018

Added a test case for when a context exceeds its max size, causing this error. It verifies the safeguard works against that.

Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

LGTM

@delner delner merged commit 32f36f0 into master Aug 6, 2018
@delner delner deleted the bugfix/prevent_http_propagation_nil_context branch August 6, 2018 15:18
@delner delner added this to the 0.13.2 milestone Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants