Add Resources to Recordable interface.#570
Conversation
lalitb
changed the title
Codecov Report
@@ Coverage Diff @@
## main #570 +/- ##
=======================================
Coverage 94.40% 94.40%
=======================================
Files 200 200
Lines 9067 9086 +19
=======================================
+ Hits 8560 8578 +18
- Misses 507 508 +1
|
| { | ||
| attribute->mutable_value()->set_bool_value(nostd::get<bool>(value)); | ||
| } | ||
|
|
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
IIUC, rather than passing resource to exporter, we're copying all the labels across spans and assuming they all share the same set?
This code was a bit harder to follow. WDYT about one of the following:
- Keep the
Resourcebundled into that type instead of using the name "resource attribtues" - Modify
MakeRecordableinterfaces to take in a resource - I'm happy to take a stab at an interface for sending resource from the TracerProvider. (Specifically I'd add a
setResourceonProcessorthat is called when a processor is added to theTracerProvider).
| auto resource = rec->GetResources(); | ||
| if (resource.attributes_size()) | ||
| { | ||
| // TODO |
| * @return the resource of this span | ||
| */ | ||
| const std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue> | ||
| &GetResources() const noexcept |
There was a problem hiding this comment.
Why doesn't this return a Resource object?
| span_kind_ = span_kind; | ||
| } | ||
|
|
||
| void SetResourceAttribute( |
There was a problem hiding this comment.
I'm trying to understand what this method is intended to do and where it is in the spec.
Fro what I gather, this is basically copying all the resource-attributes onto the span. Can we instead just grab a reference to the resource?
Thanks @jsuereth for your comments. I will prefer option 3 unless Resource is made append-only/mutable in spec. Will like to go with option 2 if that happens. Would wait for more suggestions before doing changes accordingly. |
|
@lalitb Sounds like a plan. I'll get something to you as soon as I can (this week, likely) |
|
Ok #575 is ready for comment/comparison. |
…eckout-6.x Update actions/checkout action to v6
3 participants
This is second part of initial Resource SDK implementation ( #502 ) - to propagate resources to Span exporters through Recordable interface.
TODO -> Modify otlp exporter to export resources. This is dependent on #567 , as the spans need to be grouped on basis on Resources and Instrumentation Library before exporting.
@jsuereth - It doesn't use alternate approach from processor to exporters, as that would need re-work if Resources are later made as mutable / append-only attributes ( as there are some discussion happening on this (and you are already part of that disucssions :) ) : open-telemetry/opentelemetry-specification#1298 ).
Moving to draft, as per comments from Josh.