Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(propagation): baggage support and automatic propagation #4371
feat(propagation): baggage support and automatic propagation #4371
Changes from all commits
a4d5eb2
e617a7e
9d38187
751ca10
d993b90
f7292b3
1fdf926
bcc92f9
fce842b
1482be0
bf52570
64cb8a6
4dbe74b
383eeea
5e720e5
244b42f
11c8256
166ea2e
53087f8
ec0d718
3b1d0a3
eb4bde6
9ec3406
641fc9f
7fbe7b8
5b99284
45a1518
a1a727b
087b856
8efab4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone without an in-depth knowledge of the tracing part of the library, I am wondering what other attributes of Tracing would cause the trace to be created, and also whether this method is meant to be consumed by customers or by dd-trace-rb internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since baggage specifically is supposed to be able to be added independent of a span starting, we needed to make sure we created a traceoperation if there wasn't one already available for it. I don't know of any other attributes for which this must be true. The method is meant to be consumed by customers to access the baggage object. What would you recommend doing to make that more explicit than having
# @public_api
above it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed it but I am not seeing a unit test for this method in the diff. Given that it performs custom escaping I would think a unit test would be appropriate with at least the special cases handled in the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We test inject and extract with special characters in baggage_spec.rb, is that not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff appears to mix having baggage be always present, like on this line, and optional like on line 338 below in the same file. Given that baggage is "always propagated if present" I think it would make more sense to not vivify it absent an explicit instruction, i.e. either an explicit write or the baggage came via propagation, to save on processing effort downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm so keep it nil at the start? However we'll still need to check if it's empty if a customer potentially sets a baggage item, and then removes it, we'd then have an empty hash. With regards to processing effort downstream, I don't think it would make a very large difference since as soon as inject is called for baggage we exit if the baggage value is nil or empty. Let me know if I'm misunderstanding.