Add exporter to Datadog #572

majorgreys · 2020-04-13T20:26:42Z

Add an exporter to Datadog. This implementation makes use of ddtrace to handle the creation of Datadog traces and writing them to the Datadog agent.

mauriciovasquezbernal

Few comments about details around. I tried it and I was able to see the spans in datadog.

Are the events missing?

tox.ini

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

ext/opentelemetry-ext-datadog/tests/test_datadog_exporter.py

ext/opentelemetry-ext-datadog/setup.cfg

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

docs/examples/datadog_exporter/README.rst

Kyle-Verhoog · 2020-04-21T21:45:02Z

docs/examples/datadog_exporter/README.rst

+
+.. code-block:: sh
+
+    pip install opentelemetry-api


nit: we can roll these up into a single pip install.

Not at the moment

Kyle-Verhoog · 2020-04-21T21:49:00Z

docs/examples/datadog_exporter/client.py

+
+assert len(argv) == 2
+
+with tracer.start_as_current_span("client"):


Do we want to have this example live within a while True loop with a slight delay? Might better demonstrate practical usage in the product.

Kyle-Verhoog · 2020-04-21T21:50:26Z

docs/examples/datadog_exporter/server.py

+
+
+if __name__ == "__main__":
+    app.run(port=8082)


tiny nit: might be nice to have the port easily configurable via environment variable in case there's a conflict.

Kyle-Verhoog · 2020-04-21T22:05:31Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

+        self._agent_writer = None
+
+    @property
+    def agent_writer(self):


I think this should be a private method. We keep it internal within ddtrace.

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

Kyle-Verhoog · 2020-04-21T23:01:52Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

+            else span.attributes["http.method"]
+        )
+
+    return span.attributes.get("component", span.name)


Not familiar with the otel spec, please excuse the naivety, but will component have a low-ish cardinality?

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

Kyle-Verhoog · 2020-04-21T23:12:55Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

+    def shutdown(self):
+        if self.agent_writer.started:
+            self.agent_writer.stop()
+            self.agent_writer.join(self.agent_writer.exit_timeout)


Are the otel folks cool with having a blocking operation like this? It could block for up to a second potentially. Might be worth adding a comment to state that it's required to ensure that the traces are flushed before shutdown. 🙂

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

Kyle-Verhoog · 2020-04-21T23:23:30Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/__init__.py

+
+
+def _get_span_type(span):
+    """Map span to Datadog span type"""


Are other span_types possible? Hopefully we're able to do http, web, worker, cache as well.

Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>

…er-datadog

…telemetry-python into majorgreys/exporter-datadog

…er-datadog

mauriciovasquezbernal

I looked at the span processor piece and left some comments. It's looking pretty good.

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/spanprocessor.py

mauriciovasquezbernal · 2020-05-05T15:39:43Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/spanprocessor.py

+        while self.check_traces_queue:
+            trace_id = self.check_traces_queue.pop()
+            if trace_id is self._FLUSH_TOKEN:
+                notify_flush = True


I'm wondering if this should break the loop here to do the force flushing ASAP.

I'm following here the approach in BatchExportSpanProcessor where force flushing still exports all the spans that had been queued up. Similarly, in the Datadog exporter we want to export all ready to be exported traces. Breaking the loop when we get the flush token would mean we drop ready to be exported traces. But I might have misunderstood how the batch exporter was functioning with these condition variable locks.

mauriciovasquezbernal · 2020-05-05T15:42:06Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/spanprocessor.py

+                    # check whether trace is exportable again in case that new
+                    # spans were started since we last concluded trace was
+                    # exportable


Is this case possible? Can't we consider that when all started spans are finished the trace is done and no more spans will come from the same trace?

I was thinking here of the async case where parent spans will exit before all child spans have done so.

In that case the number of started spans would be greater than the number of ended spans (there are some child spans that are not ended yet).

My point is, is the following case possible in a real scenario?

parent = tracer.start_span("parent") with tracer.use_span(parent, end_on_exit=True): with tracer.start_as_current_span("foo"): print("Hello world from OpenTelemetry Python!") # Is the trace done here? with tracer.start_as_current_span("child", parent=parent): print("here we go again")

Is it possible to start a span using a parent that is ended?

Is it possible to start a span using a parent that is ended?

Looks to be the case. The exporter output of the above are these three spans:

# "foo" Span(name="foo", context=SpanContext(trace_id=0xc979ccb3a89fa1edacef2e9bd1e6255b, span_id=0x1589d32ad80407e0, trace_state={}, is_remote=False), kind=SpanKind.INTERNAL, parent=Span(name="parent", context=SpanContext(trace_id=0xc979ccb3a89fa1edacef2e9bd1e6255b, span_id=0x4943984635edaebb, trace_state={}, is_remote=False)), start_time=2020-05-08T19:47:44.261087Z, end_time=2020-05-08T19:47:44.261138Z) # "parent" Span(name="parent", context=SpanContext(trace_id=0xc979ccb3a89fa1edacef2e9bd1e6255b, span_id=0x4943984635edaebb, trace_state={}, is_remote=False), kind=SpanKind.INTERNAL, parent=None, start_time=2020-05-08T19:47:44.260045Z, end_time=2020-05-08T19:47:44.261158Z) # child Span(name="child", context=SpanContext(trace_id=0xc979ccb3a89fa1edacef2e9bd1e6255b, span_id=0x05c69b112b2910f8, trace_state={}, is_remote=False), kind=SpanKind.INTERNAL, parent=Span(name="parent", context=SpanContext(trace_id=0xc979ccb3a89fa1edacef2e9bd1e6255b, span_id=0x4943984635edaebb, trace_state={}, is_remote=False)), start_time=2020-05-08T19:47:46.524331Z, end_time=2020-05-08T19:47:46.524393Z)

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/spanprocessor.py

mauriciovasquezbernal · 2020-05-05T15:57:21Z

ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/spanprocessor.py

+            if not self._flushing:
+                with self.condition:


The BatchSpanProcessor has a logic to avoid waiting in the condition if there are too many spans to be sent. Do you think a similar approach here could help?

The Datadog exporter does not have a notion of "max batch size" as of yet. There is a limit on the number of spans to be stored for a trace (4096) but that's handled as spans are started. At export time we export individual traces. Under the hood the writing to a Datadog agent is handled by the ddtrace library which maintains its own queue of traces and has logic around payload sizes.

…er-datadog

c24t

Changes LGTM, merging this for the 0.7b.0 release.

toumorokoshi

accepted so we can start verifying interaction with the datadog client with users. Thanks!

codeboten

I have a concern around merging this PR that I would like addressed. The spec specifically states the following here:

Other vendor-specific exporters (exporters that implement vendor protocols) should not be included in language libraries and should be placed elsewhere (the exact approach for storing and maintaining vendor-specific exporters will be defined in the future).

We asked the azure exporter be moved out specifically because of this and it came up in the stackdriver PR as well. I'd like my comment addressed before moving to approve.

codeboten · 2020-05-13T21:36:24Z

Just wanted to followup, here's the link to the azure exporter repo that was moved as per #193. I think long term we'll want all exporters to live in the contrib repo, so as long as we're ok with merging this in until we move all exporters into the contrib, then I'm fine to approve. @c24t @toumorokoshi thoughts?

c24t · 2020-05-13T21:43:15Z

so as long as we're ok with merging this in until we move all exporters into the contrib, then I'm fine to approve.

That sounds good to me. As we talked about a bit in the SIG call, it's much easier to develop exporters in the main repo since we don't have the same tests and tooling available in other repos. I think it's fine in general to develop in this repo and then move packages out once they're stable, like we did for the azure exporter.

codeboten

Created #691 to track moving the exporter code out of this repo.

Add an exporter to Datadog. This implementation makes use of ddtrace to handle the creation of Datadog traces and writing them to the Datadog agent. Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>

majorgreys force-pushed the majorgreys/exporter-datadog branch 2 times, most recently from e3a3619 to e44f29b Compare April 14, 2020 19:38

feat: Add exporter to datadog

f56b985

majorgreys force-pushed the majorgreys/exporter-datadog branch from e44f29b to f56b985 Compare April 14, 2020 19:52

majorgreys added 4 commits April 15, 2020 13:15

use component for resource

3b3e25e

add client server example

8ed8ee6

handle http resources

3d9267d

add span type

7664e6d

majorgreys force-pushed the majorgreys/exporter-datadog branch from 4405774 to 7664e6d Compare April 15, 2020 21:16

majorgreys added 3 commits April 15, 2020 17:55

add exception info

6a02800

add docs

0b74eba

fix span tests

f35291e

c24t added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Apr 16, 2020

majorgreys marked this pull request as ready for review April 16, 2020 22:51

majorgreys requested a review from a team April 16, 2020 22:51

Merge branch 'master' into majorgreys/exporter-datadog

94908ae

mauriciovasquezbernal reviewed Apr 20, 2020

View reviewed changes

Kyle-Verhoog reviewed Apr 21, 2020

View reviewed changes

c24t changed the title ~~Add exporter to Datadog~~ [WIP] Add exporter to Datadog Apr 23, 2020

majorgreys and others added 11 commits April 27, 2020 07:21

feat: add span processor for datadog

73c71a5

use assertIsNone and assertIsNotNone

6c19ce7

add note about not supporting events and links

64c8779

auto-inst client in example

480e977

Update docs/examples/datadog_exporter/README.rst

9561c66

Co-Authored-By: Mauricio Vásquez <mauricio@kinvolk.io>

Merge remote-tracking branch 'upstream/master' into majorgreys/export…

07bbeb4

…er-datadog

Merge branch 'majorgreys/exporter-datadog' of github.com:DataDog/open…

08451b3

…telemetry-python into majorgreys/exporter-datadog

only queue trace for export when all spans ended

53594b7

Merge remote-tracking branch 'upstream/master' into majorgreys/export…

9a0cabb

…er-datadog

simplify conversion to uint64

a03aec8

update examples

0459105

majorgreys changed the title ~~[WIP] Add exporter to Datadog~~ Add exporter to Datadog Apr 30, 2020

majorgreys added 2 commits April 30, 2020 17:36

Merge remote-tracking branch 'upstream/master' into majorgreys/export…

68c0e26

…er-datadog

Merge remote-tracking branch 'upstream/master' into majorgreys/export…

6e6fbbb

…er-datadog

mauriciovasquezbernal reviewed May 5, 2020

View reviewed changes

majorgreys added 8 commits May 7, 2020 16:54

add missing licenses

fe2173f

fix lint

9c451f9

cleanup lock

7d10628

updates to name, type, resource

c2e82d8

update docs

c193b36

Merge remote-tracking branch 'upstream/master' into majorgreys/export…

7f73fa1

…er-datadog

simplify exportable check

9cbe3d5

add changelog

87a8c44

c24t approved these changes May 11, 2020

View reviewed changes

toumorokoshi approved these changes May 11, 2020

View reviewed changes

majorgreys added 3 commits May 11, 2020 17:20

Merge branch 'master' into majorgreys/exporter-datadog

88d690c

Merge branch 'master' into majorgreys/exporter-datadog

7ccd8a9

Merge branch 'master' into majorgreys/exporter-datadog

471ebba

codeboten suggested changes May 13, 2020

View reviewed changes

codeboten approved these changes May 13, 2020

View reviewed changes

codeboten merged commit f1a7feb into open-telemetry:master May 13, 2020

majorgreys deleted the majorgreys/exporter-datadog branch May 13, 2020 22:14

majorgreys mentioned this pull request Jun 8, 2020

REQUEST: New membership for majorgreys open-telemetry/community#378

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exporter to Datadog #572

Add exporter to Datadog #572

majorgreys commented Apr 13, 2020

mauriciovasquezbernal left a comment

Kyle-Verhoog Apr 21, 2020

majorgreys May 8, 2020

Kyle-Verhoog Apr 21, 2020

Kyle-Verhoog Apr 21, 2020

Kyle-Verhoog Apr 21, 2020

Kyle-Verhoog Apr 21, 2020

Kyle-Verhoog Apr 21, 2020

Kyle-Verhoog Apr 21, 2020

mauriciovasquezbernal left a comment

mauriciovasquezbernal May 5, 2020

majorgreys May 8, 2020

mauriciovasquezbernal May 5, 2020

majorgreys May 8, 2020

mauriciovasquezbernal May 8, 2020

majorgreys May 8, 2020

mauriciovasquezbernal May 5, 2020

majorgreys May 8, 2020

c24t left a comment

toumorokoshi left a comment

codeboten left a comment

codeboten commented May 13, 2020

c24t commented May 13, 2020

codeboten left a comment


		assert len(argv) == 2

		with tracer.start_as_current_span("client"):



		def _get_span_type(span):
		"""Map span to Datadog span type"""

Add exporter to Datadog #572

Add exporter to Datadog #572

Conversation

majorgreys commented Apr 13, 2020

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

codeboten commented May 13, 2020

c24t commented May 13, 2020

codeboten left a comment

Choose a reason for hiding this comment