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
Propagate resource to exporters #706
Propagate resource to exporters #706
Changes from 15 commits
062bc09
44e8f05
d7d7f5a
c2307ba
c81c3b6
8477133
42dfa0e
dd694cd
56dbd3c
bfd1a32
2f02e17
420113e
e92623f
47f98fe
e8ff60f
c83f3ee
8875ce3
a5d4f13
88d914e
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.
I'm thinking here about some optimization opportunity: how often do you expect
service_name
value to change, i.e. do you have to copy it here for every recordable, then again into everyJSON
(ZipkinSpan) object?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 can think of some optimization of not passing resource and instrumentation library as part of every Recordable( and instead just passing tracer reference from where we can get both), but still, these data need to be copied to every ZipkinSpan. ( https://zipkin.io/zipkin-api/#/default/post_spans ).
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.
That's fine for now. I am looking at Zipkin exporter as example for Fluentd exporter, I can share some thoughts how I would've rearranged this in the Fluentd exporter.
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.
Thanks, will look forward to that :)
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.
Confirming re. thread-safety of
SetResource
vsGetServiceName
-- both cannot be called at once from two different threads?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.
SetResource
would be called when Span is created usingTracer::StartSpan()
. AndGetServiceName()
would be when Span is ending :Span::End() -> Processor::OnEnd() -> Exporter::Export()
. Both these need to be sequential in same thread.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.
I would let it go for now. But we may need to describe some of these expectations where we do not explicitly shield the access with a mutex, esp. in the other place where we return an object (that may get changed in another thread) by reference.
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.
Good point. Specifically for Resources, these are immutable as per the specs, with their ownership transferred to TracerProvider while it's initialization. So there won't be race condition arising while accessing them within the pipeline.