-
Notifications
You must be signed in to change notification settings - Fork 438
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
Changes from all 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,6 +213,16 @@ void Recordable::SetName(nostd::string_view name) noexcept | |
span_["name"] = name.data(); | ||
} | ||
|
||
void Recordable::SetResource(const opentelemetry::sdk::resource::Resource &resource) noexcept | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirming re. thread-safety of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
{ | ||
// only service.name attribute is supported by specs as of now. | ||
auto attributes = resource.GetAttributes(); | ||
if (attributes.find("service.name") != attributes.end()) | ||
{ | ||
service_name_ = nostd::get<std::string>(attributes["service.name"]); | ||
} | ||
} | ||
|
||
void Recordable::SetStartTime(opentelemetry::common::SystemTimestamp start_time) noexcept | ||
{ | ||
span_["timestamp"] = | ||
|
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 :)