Add stackdriver trace exporter #288

clsung · 2019-11-13T15:52:22Z

Implement #287
TODO:

add GCP/AWS/Azure labels if planned
- in OC, it detects GAE/K8S/AWS
increase test coverage

Tested on:

examples/client.py and examples/server.py
https://github.com/open-telemetry/opentelemetry-go/tree/master/example/http-stackdriver

TODO: - add GCP/AWS/Azure labels if planned - increase test coverage Test on: - examples/client.py and examples/server.py - https://github.com/open-telemetry/opentelemetry-go/tree/master/example/http-stackdriver

codecov-io · 2019-11-14T03:30:27Z

Codecov Report

Merging #288 into master will decrease coverage by 1.5%.
The diff coverage is 61.9%.

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   85.66%   84.16%   -1.51%     
==========================================
  Files          33       35       +2     
  Lines        1612     1756     +144     
  Branches      180      206      +26     
==========================================
+ Hits         1381     1478      +97     
- Misses        185      215      +30     
- Partials       46       63      +17

Impacted Files	Coverage Δ
...river/src/opentelemetry/ext/stackdriver/version.py	`100% <100%> (ø)`
...rc/opentelemetry/ext/stackdriver/trace/__init__.py	`61.6% <61.6%> (ø)`
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`83.73% <0%> (-2.56%)`	⬇️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`90.54% <0%> (-0.37%)`	⬇️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.86% <0%> (-0.1%)`	⬇️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`89.94% <0%> (+0.83%)`	⬆️
...sdk/src/opentelemetry/sdk/trace/export/__init__.py	`86.79% <0%> (+0.94%)`	⬆️
...elemetry-api/src/opentelemetry/metrics/__init__.py	`86% <0%> (+1.55%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 693391c...e1b07e7. Read the comment docs.

codeboten · 2019-11-14T16:08:41Z

Should this exporter be outside of the main opentelemetry repo?

clsung · 2019-11-15T01:56:32Z

Should this exporter be outside of the main opentelemetry repo?

Are we going to move them out now? Not sure if there's a guideline to follow.

Oberon00 · 2019-11-18T13:04:27Z

This is currently being discussed at #272 and it looks like yes, vendor-specific exporters should not be living in the main Python repository.

c24t · 2019-11-23T02:04:47Z

I think it's fine to have two PRs here: one to create the exporter and another to move in into a separate repo. We need the same reviewers in any case, and doing it this way makes it easier to write the exporter in the same style as the others, use the same style checks and CI, etc.

c24t

The direction looks good, but I've got a few blocking comments on the exporter and I think the tests need some work still.

I reviewed the code, but haven't tested against stackdriver yet. Have you tried sending traces to stackdriver? How did you test this?

It would also be helpful to list the features of the OC exporter (besides resource detection) that aren't implemented here to help reviewers compare the two.

ext/opentelemetry-ext-stackdriver/README.rst

ext/opentelemetry-ext-stackdriver/examples/client.py

ext/opentelemetry-ext-stackdriver/setup.cfg

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

ext/opentelemetry-ext-stackdriver/tests/test_stackdriver_exporter.py

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/utils.py

clsung · 2019-11-23T13:14:51Z

I reviewed the code, but haven't tested against stackdriver yet. Have you tried sending traces to stackdriver? How did you test this?

Hi, I tested it on my own GCP project, with examples/*.py (and cross-test client.py/server.py with server/client in opentelemetry-go/example/http-stackdriver)

dgzlopes

It looks good, but I found some things that might need a fix :)

dgzlopes · 2020-01-28T22:46:16Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    if not links:
+        return None
+
+    links = []


I think this won't work.

An empty list is assigned to the links function argument.

Also, single use function too.

dgzlopes · 2020-01-28T23:02:13Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+                "annotation": annotation_json,
+            }
+        )
+    return {"timeEvent": logs}


Nit.

If I have a function called extract_events I expect that it returns events, not logs.

Don't define extract_events at all, it is single use too.

dgzlopes · 2020-01-28T23:11:38Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    return (result, truncated_byte_count)
+
+
+def extract_status(status: trace_api.Status):


These functions aren't supposed to be accessed "from the outside" and should be led by an underscore (see Jaeger implementation as an example).

Even better, don't even define extract_status. Its code is only used in one place, just move it there.

Even better, don't even define this function since it is single use. Just move its code where it is needed.

Do you think it's necessary to remove all the single-use functions? Speaking because Jaeger and Zipkin exporters have some of them too (Example in Jaeger: _extract_refs_from_span _extract_logs_from_span)... And maybe they need a second look.

I don't think it is necessary, but it is very convenient 🙂

They may be present somewhere else, but that does not mean we should have them here, of course. Removing them means less code, less stuff to document, but above all, it means that when you are reading code you don't have to stop, read code somewhere else (maybe doing some mental replacement of variable names that were passed as arguments) and then come back to the original point in the code.

Makes sense. Thank you for taking the time to explain!

dgzlopes · 2020-01-28T23:23:18Z

ext/opentelemetry-ext-stackdriver/examples/client.py

+from opentelemetry.sdk.trace.export import SimpleExportSpanProcessor
+
+trace.set_preferred_tracer_implementation(lambda T: Tracer())
+tracer = trace.tracer()


I believe that this should use Named tracers work from #301. Examples in other places: Here or here.

dgzlopes · 2020-01-29T13:32:56Z

ext/opentelemetry-ext-stackdriver/tests/test_stackdriver_exporter.py

+        # durations = 50 * 10 ** 6
+        # end_times = start_times + durations
+        span_datas = [
+            Span(


I think it is necessary to test multiple spans with different fields to make the tests more reliable. Example from Jaeger translation tests.

Half of the fields right now aren't tested (startTime, endTime, parent, links...).

ocelotl · 2020-01-30T20:51:48Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+class StackdriverSpanExporter(SpanExporter):
+    """Stackdriver span exporter for OpenTelemetry.
+
+    Args:


Not sure if we are being strict on this, but PEP 257 says that the documentation for these arguments goes in the documentation for __init__.

ocelotl · 2020-01-30T20:55:55Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+
+        return SpanExportResult.SUCCESS
+
+    def translate_to_stackdriver(


Is better to avoid single call functions like this one because the reader has to look for the code here then return to this line.

ocelotl · 2020-01-30T21:01:10Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+                parent_id = "{:016x}".format(span.parent.span_id)
+
+            start_time = None
+            if span.start_time:


If span.start_time can be None use if span.start_time is not None.

ocelotl · 2020-01-30T21:02:17Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+
+logger = logging.getLogger(__name__)
+
+AGENT = "opentelemetry-python [{}]".format(__version__)


This is a single use global variable, better just move to where it is used.

ocelotl · 2020-01-30T21:04:04Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+        """Translate the spans to Stackdriver format.
+
+        Args:
+            spans: Tuple of spans to convert


A list could also be used as the spans argument.

ocelotl · 2020-01-30T21:55:31Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    return result
+
+
+def map_attributes(attribute_map):


Single use function here, too.

ocelotl · 2020-01-30T21:57:22Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    """Convert the attributes to stackdriver attributes."""
+    if attribute_map is None:
+        return attribute_map
+    for (key, value) in attribute_map.items():


attribute_map is a dictionary, Instead of looking through all of its keys (O(N)), just access "attributeMap" directly (O(1)).

ocelotl · 2020-01-30T22:02:45Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    return attribute_map
+
+
+def _format_attribute_value(value):


Single use function too. Also, just something to consider:

>>> type(1).__name__ 'int' >>> type(False).__name__ 'bool' >>> type("string").__name__ 'str' >>> type(1.1).__name__ 'float'

ocelotl · 2020-01-30T22:06:20Z

ext/opentelemetry-ext-stackdriver/src/opentelemetry/ext/stackdriver/trace/__init__.py

+    return result
+
+
+def check_str_length(str_to_check, limit=MAX_LENGTH):


You can just have 128 here instead of having a global variable named MAX_LENGTH.

ocelotl · 2020-01-30T22:14:16Z

ext/opentelemetry-ext-stackdriver/tests/test_stackdriver_exporter.py

+    def test_export(self):
+        trace_id = "6e0c63257de34c92bf9efcd03927272e"
+        span_id = "95bb5edabd45950f"
+        # start_times = 683647322 * 10 ** 9  # in ns


Please remove commented code.

lizthegrey · 2020-03-12T09:44:28Z

this should go into its own repo as it's a vendor exporter, no?

codeboten · 2020-05-21T16:27:05Z

Closed in favour of #698

* chore: update zipkin exporter README * chore: remove Prerequisites section

Add stackdriver trace exporter

75e3653

TODO: - add GCP/AWS/Azure labels if planned - increase test coverage Test on: - examples/client.py and examples/server.py - https://github.com/open-telemetry/opentelemetry-go/tree/master/example/http-stackdriver

clsung requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners November 13, 2019 15:52

clsung added 4 commits November 14, 2019 10:16

pat py3.4

850d12a

pat py3.4

3cbc0f2

missing item

086d5eb

black

3b43730

c24t suggested changes Nov 23, 2019

View reviewed changes

clsung added 7 commits November 23, 2019 19:19

fix by review

3fb85da

enrich exception, remove unused imports

caa3847

fix conflict

d4d3806

refine test

f88135b

fix example

54896ef

fix README

0dff8bc

remove span initialization in grpc

6b9b68c

toumorokoshi added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 9, 2019

remove OC header, file has changed a lot

e1b07e7

a-feld removed their request for review December 10, 2019 00:55

dgzlopes suggested changes Jan 29, 2020

View reviewed changes

ocelotl requested changes Jan 30, 2020

View reviewed changes

kintel mentioned this pull request May 20, 2020

Cloud Trace exporter #698

Merged

codeboten closed this May 21, 2020

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

chore: update zipkin exporter README (open-telemetry#288)

0881e64

* chore: update zipkin exporter README * chore: remove Prerequisites section

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stackdriver trace exporter #288

Add stackdriver trace exporter #288

clsung commented Nov 13, 2019 •

edited

Loading

codecov-io commented Nov 14, 2019 •

edited

Loading

codeboten commented Nov 14, 2019

clsung commented Nov 15, 2019

Oberon00 commented Nov 18, 2019

c24t commented Nov 23, 2019

c24t left a comment

clsung commented Nov 23, 2019

dgzlopes left a comment

dgzlopes Jan 28, 2020

ocelotl Jan 30, 2020

dgzlopes Jan 28, 2020

ocelotl Jan 30, 2020

dgzlopes Jan 28, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

dgzlopes Jan 30, 2020

ocelotl Jan 30, 2020

dgzlopes Jan 30, 2020

dgzlopes Jan 28, 2020

dgzlopes Jan 29, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

ocelotl Jan 30, 2020

lizthegrey commented Mar 12, 2020

codeboten commented May 21, 2020

		return (result, truncated_byte_count)


		def extract_status(status: trace_api.Status):


		return SpanExportResult.SUCCESS

		def translate_to_stackdriver(


		logger = logging.getLogger(__name__)

		AGENT = "opentelemetry-python [{}]".format(__version__)

		return result


		def check_str_length(str_to_check, limit=MAX_LENGTH):

Add stackdriver trace exporter #288

Add stackdriver trace exporter #288

Conversation

clsung commented Nov 13, 2019 • edited Loading

codecov-io commented Nov 14, 2019 • edited Loading

Codecov Report

codeboten commented Nov 14, 2019

clsung commented Nov 15, 2019

Oberon00 commented Nov 18, 2019

c24t commented Nov 23, 2019

c24t left a comment

Choose a reason for hiding this comment

clsung commented Nov 23, 2019

dgzlopes 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

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

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

lizthegrey commented Mar 12, 2020

codeboten commented May 21, 2020

clsung commented Nov 13, 2019 •

edited

Loading

codecov-io commented Nov 14, 2019 •

edited

Loading