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

Adds SpanOrNoopFromContext convenience function #101

Merged
merged 1 commit into from
Jan 2, 2019

Conversation

basvanbeek
Copy link
Member

This solves concerns as expressed in: #97

This typically is used in modular code where it is not guaranteed that spans are propagated in the resulting application binary. For this code it is advisable to test for nil but for those dead set on tracing API's having zero side effects this solves that issue.

@ghost ghost assigned basvanbeek Jan 2, 2019
@ghost ghost added the review label Jan 2, 2019
@basvanbeek
Copy link
Member Author

@romaindoumenc would this work for you?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 67.087% when pulling 7225a09 on basvanbeek:master into b7a66d0 on openzipkin:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 67.087% when pulling 7225a09 on basvanbeek:master into b7a66d0 on openzipkin:master.

@basvanbeek basvanbeek requested a review from jcchavezs January 2, 2019 14:00
Copy link
Contributor

@jcchavezs jcchavezs left a comment

Choose a reason for hiding this comment

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

LGTM

@romaindoumenc
Copy link

@romaindoumenc would this work for you?

Yep, and I like the documentation. Pretty sure you already thought about it, using a dedicated function makes it easy to see the progress of the conversion to Zipkin. So 👏

@basvanbeek basvanbeek merged commit f3b70f7 into openzipkin:master Jan 2, 2019
@ghost ghost removed the review label Jan 2, 2019
Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

glad to see this. if people are switching between java and go, this is similar in nature to what's served by Brave's CurrentSpanCustomizer

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