Zipkin Exporter #192

hekike · 2019-08-11T03:09:59Z

First take on Zipkin Exporter.
Please provide feedback.

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

TODO:

test coverage
batching
Event mapping (blocked by: Span Event should have a timestamp #191)
determinate span.shared

DISCUSS:

span.status mapping
shutdown behavior
retry logic / HTTP client HTTP Client for Exporters #190

mayurkale22

This is a great start :)

packages/opentelemetry-exporter-zipkin/src/types.ts

mayurkale22 · 2019-08-11T19:31:23Z

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

+   * Shutdown exporter. Noop operation in this exporter.
+   */
+  shutdown() {
+    // TODO: should we initiate an oppurtinistic send(..)?


This is a good point, I think we should call send() one last time to send last batched data.

Let's discuss behavior on the next SIG meeting. As we don't have a callback here graceful shutdown is not really possible. I'd love the spec to be more specific here about this behavior: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-exporter.md#shutdown

I think it depends on the use case. What is the use case of shutdown?

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

mayurkale22 · 2019-08-11T19:34:20Z

packages/opentelemetry-exporter-zipkin/src/transform.ts

+  [types.SpanKind.CONSUMER]: zipkinTypes.SpanKind.CONSUMER,
+  [types.SpanKind.PRODUCER]: zipkinTypes.SpanKind.PRODUCER,
+  // Zipkin doesnt have type INTERNAL
+  [types.SpanKind.INTERNAL]: zipkinTypes.SpanKind.CLIENT,


Hmm, not sure about this. Just add @todo here and will revisit again.

I think we should skip the span bcoz INTERNAL indicates that the span is used internally.

Are you sure?
SpanKind.Internal MUST be assumed as a default. - spec

This would mean that we would skip on every span by default, except someone defines SpanKind.

Hmm, I checked out Zipkin docs again and sounds like we can just leave it undefined and let JSON.stringify to remove it.

kind: When present, kind clarifies timestamp, duration and remoteEndpoint. When absent, the span is local or incomplete.

mayurkale22 · 2019-08-11T19:39:43Z

packages/opentelemetry-exporter-zipkin/src/transform.ts

+    id: span.spanContext.spanId,
+    kind: ZIPKIN_SPAN_KIND_MAPPING[span.kind],
+    timestamp: span.startTime * MICROS_PER_MILLI,
+    duration: Math.round(span.startTime - span.endTime * MICROS_PER_MILLI),


(span.endTime - span.startTime) * MICROS_PER_MILLI?

codecov-io · 2019-08-12T15:10:38Z

Codecov Report

Merging #192 into master will increase coverage by 0.09%.
The diff coverage is 99.67%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   98.86%   98.95%   +0.09%     
==========================================
  Files          69       76       +7     
  Lines        2732     3161     +429     
  Branches      193      209      +16     
==========================================
+ Hits         2701     3128     +427     
- Misses         31       33       +2

Impacted Files	Coverage Δ
...ges/opentelemetry-exporter-zipkin/test/e2e.test.ts	`100% <100%> (ø)`
...entelemetry-exporter-zipkin/test/transform.test.ts	`100% <100%> (ø)`
...ackages/opentelemetry-exporter-zipkin/src/types.ts	`100% <100%> (ø)`
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts	`100% <100%> (ø)`
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100% <100%> (ø)`
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`98.21% <98.21%> (ø)`
...pentelemetry-node-sdk/src/instrumentation/utils.ts	`96% <0%> (-4%)`	⬇️
...node-sdk/test/instrumentation/PluginLoader.test.ts	`100% <0%> (ø)`	⬆️
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts	`100% <0%> (ø)`	⬆️
... and 6 more

mayurkale22 · 2019-08-12T18:08:38Z

packages/opentelemetry-exporter-zipkin/src/transform.ts

+    id: span.spanContext.spanId,
+    kind: ZIPKIN_SPAN_KIND_MAPPING[span.kind],
+    timestamp: span.startTime * MICROS_PER_MILLI,
+    duration: (span.startTime - span.endTime) * MICROS_PER_MILLI,


I think duration should be (span.endTime - span.startTime) and not (span.startTime - span.endTime).

I still think we should store the duration and not the end time. When I think about it, it doesn't even make sense to rely on the concept of an end time since it could end up before the start time because of daylight saving time. This makes the current implementation semantically incorrect.

Regarding daylight savings time, my understanding is that performance.now is monotonic, so that wouldn't be an issue if we use it? Or am I missing something?

I'm open to storing the duration, but thought it could be simpler to just store start and end since that's what's provided either at measurement time or as a parameter

mayurkale22 · 2019-08-15T00:01:49Z

Test coverage

Please include the tests when you get time.

Batching

The Batching will most likely be handled by the SpanProcessor open-telemetry/opentelemetry-specification#205, I have PR(#149) open on same. Please add comments on the specs issue.

Event mapping

Handled in #195, should un-block you.

determinate span.shared

I think keep it false for now and add @todo and create an issue to address it later.

span.status mapping

Per SIG discussion, let's go with current mapping (ot.status_code), will revisit later. Moving forward, will share clear guidance on changes from OpenCensus to OpenTelemetry.

shutdown behavior

Per SIG discussion, make it configurable shutdown(forceFlush: boolean = true). If true, it will do (non-blocking) flush data and graceful shutdown otherwise direct exit. I got to know that callback is not required in this case, the SDK should do the best job to flush out the data - Log the warn/error if it is really required.

retry logic / HTTP client #190

Per SIG discussion, this is up-to exporter to implement the retry mechanism. Use raw right now and it’s sufficient.

packages/opentelemetry-basic-tracer/src/Span.ts

hekike · 2019-08-17T17:19:53Z

@mayurkale22 I added tests and tested with Zipkin backend. Could also make some fields optional when not provided to minimize the payload. After integrating the batch logic, it's ready to go.

packages/opentelemetry-exporter-zipkin/src/transform.ts

packages/opentelemetry-exporter-zipkin/package.json

mayurkale22 · 2019-08-17T18:13:06Z

packages/opentelemetry-exporter-zipkin/src/types.ts

+  url?: string;
+  // Initiates a request with spans in memory to the backend.
+  forceFlush?: boolean;
+  // Optional mapping overrides for OpenTelemetry status code and description.


Good idea :)

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

packages/opentelemetry-exporter-zipkin/package.json

mayurkale22

LGTM, added a minor comment. Thanks for the work!

packages/opentelemetry-exporter-zipkin/test/zipkin.test.ts

hekike · 2019-08-18T19:46:02Z

@mayurkale22 thanks for the review, I changed it.
Also opened an issue open-telemetry/opentelemetry-specification#220 and PR open-telemetry/opentelemetry-specification#221 in the spec repo about Zipkin mapping.

jcchavezs · 2019-09-10T18:04:32Z

packages/opentelemetry-exporter-zipkin/src/types.ts

+/**
+ * The network context of a node in the service graph.
+ */
+export interface Endpoint {


Zipkin format also supports ipv6 for endpoints. Any particular reason for not including it?

Yes, you're right. This is what I found on zipkin API.

description: The network context of a node in the service graph

serviceName stringLower-case label of this node in the service graph, such as "favstar". Leaveabsent if unknown.This is a primary label for trace lookup and aggregation, so it should beintuitive and consistent. Many use a name from service discovery.

ipv4 string($ipv4)The text representation of the primary IPv4 address associated with thisconnection. Ex. 192.168.99.100 Absent if unknown.

ipv6 string($ipv6)The text representation of the primary IPv6 address associated with aconnection. Ex. 2001:db8::c001 Absent if unknown.Prefer using the ipv4 field for mapped addresses.

port integerDepending on context, this could be a listen port or the client-side of asocket. Absent if unknown. Please don’t set to zero.

@hekike Could you please add support ipv6? Thanks

I'd expect ipv6 would be handled by node, do I miss something?

$ node -p -e 'require("url").parse("http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:9411/api/v2/spans")' Url { protocol: 'http:', slashes: true, auth: null, host: '[fedc:ba98:7654:3210:fedc:ba98:7654:3210]:9411', port: '9411', hostname: 'fedc:ba98:7654:3210:fedc:ba98:7654:3210', hash: null, search: null, query: null, pathname: '/api/v2/spans', path: '/api/v2/spans', href: 'http://[fedc:ba98:7654:3210:fedc:ba98:7654:3210]:9411/api/v2/spans' }

hekike · 2019-09-18T15:47:06Z

@mayurkale22 PR rebased and I think ready to go.

mayurkale22 · 2019-09-18T16:44:22Z

@mayurkale22 PR rebased and I think ready to go.

Great, I will take a look today and merge once everything looks good. Thanks for the work @hekike

mayurkale22 · 2019-09-18T20:50:48Z

@open-telemetry/node-approvers Please review when you get time, thanks :)

mayurkale22 · 2019-09-19T17:50:10Z

@hekike thanks for the great work! I will merge this PR now and run end-to-end test.

* chore: use addSpanNetworkEvents from otel-web Rather than use a modified copy, adjust event creation code to use shared routine from otel-web. This has the pleasant side effect of also adding http.response_content_length. * fix: bring resource fetch spans into spec compliance Names should not be raw URLs, so I arbitrarily chose 'resourceFetch' to mirror 'documentFetch' - I'm open to any better suggestions. Because the resource url is of course very useful I've put it in the 'http.url' attribute instead. * chore: lint:fix and use semantic constants * fix: firefox sometimes has a fetchStart of 0, doesn't emit doc load spans I noticed that in unit and manual tests Firefox would frequently not emit doc load events. Turns out that sometimes the fetchStart was 0 and the logic prevented spans from being emitted. Simply checking >= 0 rather than > 0 fixes that. * chore: use constant for resourceFetch span name Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

hekike requested review from danielkhan, iredelmeier, mayurkale22 and rochdev as code owners August 11, 2019 03:10

hekike mentioned this pull request Aug 11, 2019

SDK: Implement Zipkin exporter #174

Closed

mayurkale22 requested a review from bg451 August 11, 2019 19:11

mayurkale22 reviewed Aug 11, 2019

View reviewed changes

mayurkale22 reviewed Aug 12, 2019

View reviewed changes

mayurkale22 mentioned this pull request Aug 14, 2019

fix(basic-tracer): add timestamp for event #195

Merged

hekike force-pushed the feat/zipkin-exporter branch from 77d8a4a to 945cfe1 Compare August 17, 2019 02:46

hekike commented Aug 17, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/src/Span.ts Outdated Show resolved Hide resolved

mayurkale22 reviewed Aug 17, 2019

View reviewed changes

hekike mentioned this pull request Aug 18, 2019

Zipkin exporter mappings open-telemetry/opentelemetry-specification#220

Closed

mayurkale22 approved these changes Aug 18, 2019

View reviewed changes

packages/opentelemetry-exporter-zipkin/test/zipkin.test.ts Show resolved Hide resolved

mayurkale22 mentioned this pull request Sep 5, 2019

feat: add InMemorySpanExporter, MultiSpanProcessor #234

Merged

jcchavezs reviewed Sep 10, 2019

View reviewed changes

hekike force-pushed the feat/zipkin-exporter branch from b625ca7 to d58ff55 Compare September 18, 2019 15:41

hekike requested review from OlivierAlbertini and vmarchaud as code owners September 18, 2019 15:41

hekike force-pushed the feat/zipkin-exporter branch from d58ff55 to 510b2b5 Compare September 18, 2019 15:44

Peter Marton added 3 commits September 18, 2019 13:50

feat(zipkin-exporter): implement

f200f5f

fix(zipkin): use time fns

9aab237

refactor(exporter-zipkin): linter fix

cc5432e

hekike force-pushed the feat/zipkin-exporter branch from 4e6421a to cc5432e Compare September 18, 2019 21:01

Merge branch 'master' into feat/zipkin-exporter

214f591

mayurkale22 merged commit b58965c into open-telemetry:master Sep 19, 2019

codeboten mentioned this pull request Dec 6, 2019

Adding Zipkin exporter open-telemetry/opentelemetry-python#320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zipkin Exporter #192

Zipkin Exporter #192

hekike commented Aug 11, 2019 •

edited

Loading

mayurkale22 left a comment

mayurkale22 Aug 11, 2019

hekike Aug 12, 2019

rochdev Aug 14, 2019

mayurkale22 Aug 11, 2019

mayurkale22 Aug 12, 2019

hekike Aug 17, 2019 •

edited

Loading

hekike Aug 17, 2019

mayurkale22 Aug 11, 2019

codecov-io commented Aug 12, 2019 •

edited

Loading

mayurkale22 Aug 12, 2019

rochdev Aug 14, 2019

draffensperger Aug 14, 2019

mayurkale22 commented Aug 15, 2019

hekike commented Aug 17, 2019 •

edited

Loading

mayurkale22 Aug 17, 2019

mayurkale22 left a comment

hekike commented Aug 18, 2019

jcchavezs Sep 10, 2019 •

edited

Loading

mayurkale22 Sep 10, 2019

hekike Sep 18, 2019

hekike commented Sep 18, 2019

mayurkale22 commented Sep 18, 2019

mayurkale22 commented Sep 18, 2019

mayurkale22 commented Sep 19, 2019

description:	The network context of a node in the service graph
serviceName	stringLower-case label of this node in the service graph, such as "favstar". Leaveabsent if unknown.This is a primary label for trace lookup and aggregation, so it should beintuitive and consistent. Many use a name from service discovery.
ipv4	string($ipv4)The text representation of the primary IPv4 address associated with thisconnection. Ex. 192.168.99.100 Absent if unknown.
ipv6	string($ipv6)The text representation of the primary IPv6 address associated with aconnection. Ex. 2001:db8::c001 Absent if unknown.Prefer using the ipv4 field for mapped addresses.
port	integerDepending on context, this could be a listen port or the client-side of asocket. Absent if unknown. Please don’t set to zero.

Zipkin Exporter #192

Zipkin Exporter #192

Conversation

hekike commented Aug 11, 2019 • edited Loading

mayurkale22 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

hekike Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 12, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Aug 15, 2019

hekike commented Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

hekike commented Aug 18, 2019

jcchavezs Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike commented Sep 18, 2019

mayurkale22 commented Sep 18, 2019

mayurkale22 commented Sep 18, 2019

mayurkale22 commented Sep 19, 2019

hekike commented Aug 11, 2019 •

edited

Loading

hekike Aug 17, 2019 •

edited

Loading

codecov-io commented Aug 12, 2019 •

edited

Loading

hekike commented Aug 17, 2019 •

edited

Loading

jcchavezs Sep 10, 2019 •

edited

Loading