Skip to content

Add interfaces for exporter, processor and span data#55

Merged
reyang merged 19 commits intoopen-telemetry:masterfrom
pyohannes:exporter-interface
Apr 20, 2020
Merged

Add interfaces for exporter, processor and span data#55
reyang merged 19 commits intoopen-telemetry:masterfrom
pyohannes:exporter-interface

Conversation

@pyohannes
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes commented Mar 20, 2020

This PR adds:

  • interfaces for exporters and processors
  • an implementation of the simple span processor required by the spec,
  • an implementation of the default SpanData recordable.

Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/exporter.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/exporter.h Outdated
Comment thread sdk/src/trace/tracer.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated

// The end epoch timestamp in nanos of this span.
virtual opentelemetry::core::SystemTimestamp GetEndTime() const noexcept = 0;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Events? Is that TODO?

Copy link
Copy Markdown
Contributor Author

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.

Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
@pyohannes pyohannes force-pushed the exporter-interface branch from deb76ed to 8925bea Compare March 25, 2020 04:45
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
@pyohannes
Copy link
Copy Markdown
Contributor Author

As discussed on Monday, I added implementations to test the interfaces:

Comment thread sdk/src/trace/simple_processor.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/exporter.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/exporter.h Outdated
@pyohannes pyohannes force-pushed the exporter-interface branch from ef07670 to 0f5c4e9 Compare April 6, 2020 21:03
* 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;
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Comment thread sdk/include/opentelemetry/sdk/trace/span_data.h Outdated
Comment thread sdk/test/trace/BUILD Outdated
)

cc_test(
name = "span_data",
Copy link
Copy Markdown
Contributor

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

Comment thread sdk/test/trace/BUILD Outdated
)

cc_test(
name = "simple_processor",
Copy link
Copy Markdown
Contributor

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/

@rnburn
Copy link
Copy Markdown
Contributor

rnburn commented Apr 13, 2020

Looks good. I think we'll need some tweaks to the exporter to

  1. Work with span circular buffers where the data isn't stored in a single contiguous segment.
  2. Support forking (needs something like a restart function)
  3. Support streaming exporters.

But I'm good merging in and evolving.

Comment thread sdk/test/trace/simple_processor_test.cc
@pyohannes
Copy link
Copy Markdown
Contributor Author

I added a new issue for the remaining open points: #62

With this in mind, I think this PR can be merged.

Copy link
Copy Markdown
Member

@bogdandrutu bogdandrutu left a 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;
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor

@rnburn rnburn Apr 14, 2020

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;
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

@pyohannes pyohannes Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. For non-optimized cases, the exporter can return a SpanData instance, which derives from Recordable.

There are good descriptions and discussions about this approach in this PR. It's shown in action here.

Comment on lines 24 to +26
virtual ~Recordable() = default;

/**
Copy link
Copy Markdown
Member

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.

@pyohannes pyohannes force-pushed the exporter-interface branch from a1bbf08 to c4c1f09 Compare April 20, 2020 22:02
@reyang reyang merged commit 9e2418b into open-telemetry:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants