sdk: add Exporter interface, SimpleSpanProcessor and InMemorySpanExporter by mauriciovasquezbernal · Pull Request #119 · open-telemetry/opentelemetry-python

mauriciovasquezbernal · 2019-08-30T21:54:35Z

This PR is a follow up of #115

It adds the Exporter interface, the SimpleSpanProcessor as described in the specs: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-exporter.md and an InMemorySpanExporter class that implements the interface and saves Spans in a list.

The following is an example of how the SpanExporter interface could be used:

from opentelemetry.sdk import trace
from opentelemetry.sdk.trace import export

# implement `SpanExporter` interface
class MySpanExporter(export.SpanExporter):
    def export(self, spans: trace.Span) -> export.SpanExportResult:
        print("export span...")
        return export.SpanExportResult.SUCCESS


tracer = trace.Tracer()

# create exporter instance
exporter = MySpanExporter()

# create SimpleExportSpanProcessor
span_processor = export.SimpleExportSpanProcessor(exporter)

# add span processor to tracer
tracer.add_span_processor(span_processor)

# create some spans for testing
with tracer.start_span("foo"):
    with tracer.start_span("bar"):
        with tracer.start_span("baz"):
            pass

Finally, the following is an example of the InMemorySpanExporter usage:

from opentelemetry.sdk import trace
from opentelemetry.sdk.trace import export
from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter

tracer = trace.Tracer()

# create in memory span exporter
in_memory_exporter = InMemorySpanExporter()

# create SimpleExportSpanProcessor
span_processor = export.SimpleExportSpanProcessor(in_memory_exporter)

# add span processor to tracer
tracer.add_span_processor(span_processor)

# craete some spans for testing
with tracer.start_span("foo"):
    with tracer.start_span("bar"):
        with tracer.start_span("baz"):
            pass

print("exported spans are: ")
spans = in_memory_exporter.get_finished_spans()
for span in spans:
    print(span)

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/in_memory_span_exporter.py

opentelemetry-sdk/tests/trace/export/test_export.py

mauriciovasquezbernal · 2019-09-03T13:10:56Z

Given that we are already talking about names, should it be called Exporter or SpanExporter?
Spec says Exporter: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-exporter.md
Java uses SpanExporter: https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/src/main/java/io/opentelemetry/sdk/trace/export/SpanExporter.java

carlosalberto · 2019-09-03T13:19:19Z

I'd suggest SpanExporter (Go also uses this name).

On a related note, maybe we need to update this Specification part ;)

mauriciovasquezbernal · 2019-09-03T13:27:30Z

Sorry Carlos, I think go is also using Export -> https://github.com/open-telemetry/opentelemetry-go/blob/3fc6025071d5a6590501939dd3271b700ba01c14/sdk/trace/export.go#L34.
I also agree that SpanExporter is a better name though.

Oberon00 · 2019-09-03T13:34:43Z

+1 for SpanExporter (I have no strong opinion though)

carlosalberto · 2019-09-03T15:39:05Z

Oh, interesting. Go used to use SpanExporter, bah. I personally still prefer SpanContext. We can later change it depending on whether or not we update the Specification itself (will fill up an issue for that).

Oberon00

I'm approving this already to speed things up, but the logging issue should really be fixed before merging.

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/in_memory_span_exporter.py

mauriciovasquezbernal · 2019-09-11T20:44:22Z

I pushed a new commit to expose span members using properties. @Oberon00 I kindly as you to check if your approval is still valid after this update.

c24t

3d8f4fa seems like it belongs in a different PR, but I have no blocking comments otherwise. It SGTM to merge this and change the relationship between processors and exporters in another PR as needed.

c24t · 2019-09-12T01:28:10Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

+        """
+
+
+class SimpleExportSpanProcessor(SpanProcessor):


Why put this in the export module instead of with other processors?

This SimpleExportSpanProcessor is strictly related to the exporters it is the glue code between SpanProcessor and Exporter. For this reason I decided to place it there.

c24t · 2019-09-12T01:28:57Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

+        self.span_exporter = span_exporter
+
+    def on_start(self, span: Span) -> None:
+        pass  # do nothing


Nit: I think it's a safe bet that people will know that pass does nothing.

Definitely it is.

c24t · 2019-09-12T01:29:38Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/in_memory_span_exporter.py

+    """Implementation of :class:`.Exporter` that stores spans in memory.
+
+    This class can be used for testing purposes. It stores the exported spans
+    in a list in memory that can be retrieve using the


Suggested change

in a list in memory that can be retrieve using the

in a list in memory that can be retrieved using the

c24t · 2019-09-12T01:53:20Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/in_memory_span_exporter.py

+    def export(self, spans: typing.Sequence[Span]) -> SpanExportResult:
+        """Stores a list of spans in memory."""
+        if self._stopped:
+            return SpanExportResult.FAILED_NOT_RETRYABLE


We say that the API shouldn't prescribe throwing exceptions, and that implementations of the API shouldn't throw either, but what about methods like this that only exist in the SDK? This does look like the right approach to me, but I don't know why exporters and processors aren't in the API in the first place.

They aren't in the API because if you create a specialized implementation of the API, you probably won't need to provide as much customization as the generic SDK. At least that's my understanding.

c24t · 2019-09-12T01:58:38Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+
+    @property
+    def parent(self) -> trace_api.ParentSpan:
+        return self._parent


3d8f4fa looks to me like a lot of boilerplate for little value. Should every attribute in the SDK be hidden behind a property? If the property decorator signals that the attribute is meant to be read-only, should name be a regular attribute since there's a trivial setter? What about the attributes without getters here?

I'm also not a fan of such trivial Java-like boilerplate properties.

OK, I am removing that then. For now we'll keep passing the the whole Span to the exporters, later on we can improve it by passing a readonly span, a SpanView or something similar.

c24t · 2019-09-12T02:03:24Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

+
+
+class SimpleExportSpanProcessor(SpanProcessor):
+    """An implementation of `SpanProcessor` that passes ended spans directly


Style nit: I think it helps readability for docstrings to have a short first line and longer wrapping text in another line below.

From #61 (review):

"""A short, single-line description of the function. A longer description that may include :class:`.SphinxStuff` and can wrap and wrap and wrap. Args: arg: Short description, no need to include the type. Returns: The thing we return, or "Yields:" for context managers. """

Done!. Thanks.

Need to look again

Exporter is an interface that allows different services to export recorded spans in its own format. SimpleExportSpanProcessor is an implementation of SpanProcessor that passes ended spans directly to a configured Exporter. The current interface for exporters directly receives an SDK Span, it could be improved in the future to receive a different object containing a representation of the Span.

InMemorySpanExporter is a simple implementation of the Exporter interface that saves the exported spans in a list in memory. This class is useful for testing purposes.

Oberon00

Due to the force push, I have no idea what actually changed, but I see that the span properties have been removed, so I re-approve, trusting that the force push changed nothing else of relevance.

mauriciovasquezbernal requested review from Oberon00, a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners August 30, 2019 21:54

reyang reviewed Aug 30, 2019

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py Outdated Show resolved Hide resolved

Oberon00 suggested changes Sep 2, 2019

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/add_exporters branch 2 times, most recently from 423346a to fc0bfd9 Compare September 2, 2019 21:15

mauriciovasquezbernal changed the title ~~sdk: add Exporter interface, SimpleSpanProcessor and InMemoySpanExporter~~ sdk: add Exporter interface, SimpleSpanProcessor and InMemorySpanExporter Sep 2, 2019

mauriciovasquezbernal force-pushed the mauricio/add_exporters branch from fc0bfd9 to 624ac51 Compare September 3, 2019 14:46

mauriciovasquezbernal force-pushed the mauricio/add_exporters branch 3 times, most recently from 72d2031 to e864c58 Compare September 5, 2019 19:40

Oberon00 previously approved these changes Sep 6, 2019

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py Outdated Show resolved Hide resolved

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/in_memory_span_exporter.py Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/add_exporters branch 2 times, most recently from 3f2f63e to 139e960 Compare September 6, 2019 13:08

c24t approved these changes Sep 12, 2019

View reviewed changes

c24t reviewed Sep 12, 2019

View reviewed changes

Oberon00 self-requested a review September 12, 2019 08:58

sdk/trace/exporter: add InMemorySpanExporter

f4ef8eb

InMemorySpanExporter is a simple implementation of the Exporter interface that saves the exported spans in a list in memory. This class is useful for testing purposes.

mauriciovasquezbernal force-pushed the mauricio/add_exporters branch from 3d8f4fa to f4ef8eb Compare September 12, 2019 14:32

Oberon00 approved these changes Sep 12, 2019

View reviewed changes

Oberon00 added this to the Alpha Release milestone Sep 13, 2019

Oberon00 merged commit d5e5471 into open-telemetry:master Sep 13, 2019

Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-python that referenced this pull request Sep 13, 2019

Fix lint build after merging open-telemetry#119.

379f85e

Oberon00 mentioned this pull request Sep 13, 2019

Fix lint build after merging #130. #135

Merged

mauriciovasquezbernal mentioned this pull request Sep 18, 2019

Implement BatchSpanProcessorExporter #149

Closed

mauriciovasquezbernal deleted the mauricio/add_exporters branch April 14, 2020 21:50

	in a list in memory that can be retrieve using the
	in a list in memory that can be retrieved using the



		class SimpleExportSpanProcessor(SpanProcessor):
		"""An implementation of `SpanProcessor` that passes ended spans directly

Conversation

mauriciovasquezbernal commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal commented Sep 3, 2019

Uh oh!

carlosalberto commented Sep 3, 2019

Uh oh!

mauriciovasquezbernal commented Sep 3, 2019

Uh oh!

Oberon00 commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosalberto commented Sep 3, 2019

Uh oh!

Oberon00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal commented Sep 11, 2019

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oberon00 Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Oberon00 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mauriciovasquezbernal commented Aug 30, 2019 •

edited

Loading

Oberon00 commented Sep 3, 2019 •

edited

Loading

Oberon00 Sep 12, 2019 •

edited

Loading