Adding Zipkin exporter by codeboten · Pull Request #320 · open-telemetry/opentelemetry-python

codeboten · 2019-12-06T19:40:00Z

Fixes #318

Pulled some of the code from the OC implementation and looked at the javascript implementation

Not addressed yet:

what do to on failures beyond returning NOT_RETRYABLE

Zipkin Spec:
https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml

Signed-off-by: Alex Boten aboten@lightstep.com

Signed-off-by: Alex Boten <aboten@lightstep.com>

codecov-io · 2019-12-06T19:49:58Z

Codecov Report

❌ Patch coverage is 84.41558% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.82%. Comparing base (8fa21e6) to head (b20310c).

Files with missing lines	Patch %	Lines
...xt-zipkin/src/opentelemetry/ext/zipkin/__init__.py	84.41%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
+ Coverage   84.73%   84.82%   +0.09%     
==========================================
  Files          35       38       +3     
  Lines        1716     1839     +123     
  Branches      200      217      +17     
==========================================
+ Hits         1454     1560     +106     
- Misses        204      214      +10     
- Partials       58       65       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

c24t

This looks great, some minor comments only.

tox.ini

ext/opentelemetry-ext-zipkin/tests/test_zipkin_exporter.py

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

Signed-off-by: Alex Boten <aboten@lightstep.com>

c24t · 2019-12-17T21:42:06Z

Recent changes LGTM too, cc @open-telemetry/python-approvers to get another review on this.

hectorhdzg · 2019-12-20T00:09:01Z

Would be good to add README with installation/usage instructions like other ext packages

toumorokoshi

Nice! looks great. some nice to haves to fix, but it looks good to me functionally.

toumorokoshi · 2019-12-20T04:31:58Z

ext/opentelemetry-ext-zipkin/setup.cfg

+    =src
+packages=find_namespace:
+install_requires =
+    requests~=2.7


should we enforce this on the package? I have a feeling it will make it hard for people to adopt.

what would you recommend here?

nevermind, I think I forgot the usage of the compatible release qualifier. This looks good.

toumorokoshi · 2019-12-20T04:34:15Z

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

+        self.ipv4 = ipv4
+        self.ipv6 = ipv6
+
+    def export(self, spans: Sequence[Span]) -> "SpanExportResult":


any need to reference this by a string vs class directly?

There is not, updated.

toumorokoshi · 2019-12-20T04:35:38Z

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

+
+    def export(self, spans: Sequence[Span]) -> "SpanExportResult":
+        zipkin_spans = self._translate_to_zipkin(spans)
+        result = requests.post(


general question: does the SpanExporterer have any handling for when an export takes too long?

I was going to suggest putting a timeout on this request, but it seems like, to keep the export working, we would need to provide a more general timeout mechanism, potentially in the caller to export.

Good point, i don't know that the exporter currently does anything here. From some quick testing, it looks like the exporter could hang indefinitely waiting for an export to complete.

toumorokoshi · 2019-12-20T04:36:29Z

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

+    SpanKind.CONSUMER: "CONSUMER",
+}
+
+SUCCESS_STATUS_CODE = (200, 202)


nitpick: "SUCCESS_STATUS_CODE" implies a single int. would be nice to callout a plural (CODES) or a CODE_LIST.

good catch, updated to CODES

toumorokoshi · 2019-12-20T04:37:15Z

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

+                result.status_code,
+                result.text,
+            )
+            # TODO: should we retry here?


maybe it's good as a configuration variable?

good idea, i added a retry option to configure the behaviour

toumorokoshi · 2019-12-20T04:48:06Z

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

+        pass
+
+
+def _format(unformatted_id):


this seems like a fairly small method. is it worth keeping?

toumorokoshi · 2019-12-20T04:49:30Z

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

+        return None
+    tags = {}
+    for attribute_key, attribute_value in attr.items():
+        if isinstance(attribute_value, (int, bool, float)):


is this case allowed? it seems like we should do some enforcement on the creation of attributes spans, rather than try to catch it on the flush side here.

Attributes are assigned one of the following values:

opentelemetry-python/opentelemetry-api/src/opentelemetry/util/types.py

Line 18 in 531d0b1

AttributeValue = typing.Union[str, bool, float]

Looks like int is not one of them though, i'll remove it.

Although that isn't enforced at this point... I can create a separate ticket for this

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

Line 211 in baab508

self.attributes[key] = value

I'm in issues anyway: #347. Thanks!

toumorokoshi · 2019-12-20T04:50:05Z

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

+        if isinstance(attribute_value, (int, bool, float)):
+            value = str(attribute_value)
+        elif isinstance(attribute_value, str):
+            res = attribute_value[:128]


is the "res" variable needed?

it is not. good catch. removed

toumorokoshi · 2019-12-20T04:50:54Z

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

+def _extract_annotations_from_events(events):
+    if not events:
+        return None
+    annotations = []


nitpick: this could be a fairly intuitive list comprehension.

toumorokoshi · 2019-12-20T04:57:16Z

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

+            start_timestamp_mus = _nsec_to_usec_round(span.start_time)
+            duration_mus = _nsec_to_usec_round(span.end_time - span.start_time)
+
+            zipkin_span = {


it looks like there's no handling of the "debug" field here, which could be extracted from the boolean flags in the TraceOptions.

theoretically we could include debug as a bit in trace_options, or use the sampled bit and consider that debug: https://www.w3.org/TR/trace-context-1/#trace-flags

i didn't see anything referencing a debug field in the zipkin API https://zipkin.io/zipkin-api/#/default/post_spans. can you elaborate on this?

Documentation is in that page under Models - Span

https://zipkin.io/zipkin-api/#/default/post_spans

debug boolean True is a request to store this span even if it overrides sampling policy.This is true when the X-B3-Flags header has a value of 1.

Easy enough to use the sampling bit.

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten · 2019-12-20T16:21:53Z

Would be good to add README with installation/usage instructions like other ext packages

Added

Signed-off-by: Alex Boten <aboten@lightstep.com>

- adding retry configuration option - adding link to zipkin documentation for microseconds Signed-off-by: Alex Boten <aboten@lightstep.com>

Signed-off-by: Alex Boten <aboten@lightstep.com>

toumorokoshi · 2019-12-30T06:03:07Z

Amazing! Thanks.

adding zipkin exporter

28c6463

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten requested review from Oberon00, a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners December 6, 2019 19:40

add coverage for zipkin and flask

ec63dec

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

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

c24t approved these changes Dec 10, 2019

View reviewed changes

Alex Boten added 2 commits December 16, 2019 13:52

Merge remote-tracking branch 'origin/master' into issue-318

01dfed0

cleaning up copyright

9d8d807

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten requested a review from a team December 16, 2019 21:56

Alex Boten added 3 commits December 16, 2019 14:02

removing get_url

c3e415e

remove Optional from required params

cb06b40

Signed-off-by: Alex Boten <aboten@lightstep.com>

forgot to save tox.ini

93331ff

Signed-off-by: Alex Boten <aboten@lightstep.com>

toumorokoshi approved these changes Dec 20, 2019

View reviewed changes

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

adding readme + changelog

e93e7b0

Signed-off-by: Alex Boten <aboten@lightstep.com>

Alex Boten added 3 commits December 20, 2019 08:23

minor fix to readme

8946f4a

Signed-off-by: Alex Boten <aboten@lightstep.com>

code cleanup

0e21ed8

Signed-off-by: Alex Boten <aboten@lightstep.com>

code cleanup

4736004

- adding retry configuration option - adding link to zipkin documentation for microseconds Signed-off-by: Alex Boten <aboten@lightstep.com>

hectorhdzg approved these changes Dec 20, 2019

View reviewed changes

add debug field

b20310c

Signed-off-by: Alex Boten <aboten@lightstep.com>

toumorokoshi merged commit 4458698 into open-telemetry:master Dec 30, 2019

codeboten deleted the issue-318 branch October 9, 2020 18:50

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

chore: remove deprecated dependency (open-telemetry#320)

0024d0a

Conversation

codeboten commented Dec 6, 2019

Uh oh!

codecov-io commented Dec 6, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

c24t commented Dec 17, 2019

Uh oh!

hectorhdzg commented Dec 20, 2019

Uh oh!

toumorokoshi 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

hectorhdzg Dec 20, 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!

codeboten commented Dec 20, 2019

Uh oh!

toumorokoshi commented Dec 30, 2019

Uh oh!

codecov-io commented Dec 6, 2019 •

edited by codecov bot

Loading

hectorhdzg Dec 20, 2019 •

edited

Loading