-
Notifications
You must be signed in to change notification settings - Fork 417
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
Add interfaces for exporter, processor and span data #55
Conversation
|
||
// The end epoch timestamp in nanos of this span. | ||
virtual opentelemetry::core::SystemTimestamp GetEndTime() const noexcept = 0; | ||
}; |
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.
Events? Is that TODO?
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.
Yes, for now I left events (as well as attributes, links and resources) out on purpose. I'd like to handle that in a separate PR.
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.
LGTM.
deb76ed
to
8925bea
Compare
As discussed on Monday, I added implementations to test the interfaces:
|
ef07670
to
0f5c4e9
Compare
* Set a parent span id for this span. | ||
* @param parent_span_id the parent span id to set | ||
*/ | ||
virtual void SetParentSpanId(opentelemetry::trace::SpanId parent_span_id) noexcept = 0; |
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.
Could we combine SetTraceId, SetSpanId, SetParentSpanId into something like SetSpanContext(trace_id, span_id, parent_id)? For eager serialization implementations, it's common for them to be written together as a group.
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.
Makes sense. I think SetSpanContext
is a bit ambiguous, as the concept of SpanContext has a different meaning int the spec. I went with SetIds
instead.
sdk/test/trace/BUILD
Outdated
@@ -8,3 +8,25 @@ cc_test( | |||
"@com_google_googletest//:gtest_main", | |||
], | |||
) | |||
|
|||
cc_test( | |||
name = "span_data", |
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 convention we've been using is to name tests with a "_test" suffix (e.g. trace_id_test, span_data_test). See, for example,
https://github.com/open-telemetry/opentelemetry-cpp/blob/master/api/test/trace/BUILD#L42-L51
sdk/test/trace/BUILD
Outdated
) | ||
|
||
cc_test( | ||
name = "simple_processor", |
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.
s/simple_processor/simple_processor_test/
Looks good. I think we'll need some tweaks to the exporter to
But I'm good merging in and evolving. |
I added a new issue for the remaining open points: #62 With this in mind, I think this PR can be merged. |
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.
Some initial feedback about exporters and span data.
* SDK. | ||
* @return a newly initialized Recordable object | ||
*/ | ||
virtual std::unique_ptr<Recordable> MakeRecordable() noexcept = 0; |
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.
Why is this in the interface? Do I need to implement this function in order to implement an 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.
That's to allow for more efficient implementations.
It's expensive to build up a SpanData-like object that provides an accessor interface. Having this as a customization point, allows for direct serialization approaches where memory is allocated in larger blocks and the serialization is built up eagerly as methods on the tracer are called (I outlined the approach in #2)
See, for example, this benchmark from lightstep's cpp tracer where we compare a custom eager serialization approach (manual) to a SpanData-like appraoch that uses protobuf-generated objects (legacy_manual). The results show that the cost can be substantially less (ranges from 2x-5x in that run). If you look at the profiles (manual vs legacy_manual), you'll see the eager serialization approach saves a lot from avoiding small memory allocations.
But there's a SpanData Recordable in the SDK, so if an implementer doesn't want to provide their own Recordable, they can write MakeRecordable as a stub that returns a SpanData.
* associated exporter. | ||
* @return a newly initialized recordable | ||
*/ | ||
virtual std::unique_ptr<Recordable> MakeRecordable() noexcept = 0; |
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.
Why is this in the interface? Do I need to implement this function in order to implement an 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.
virtual ~Recordable() = default; | ||
|
||
/** |
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.
In other languages Recordable
is not an interface but a concrete immutable class.
a1bbf08
to
c4c1f09
Compare
This PR adds:
SpanData
recordable.