feat: Add OpenTracing shim #212

bg451 · 2019-08-22T17:28:20Z

Resolves #181

In order to make the transition to OpenTelemetry easy for existing OpenCensus and Opentracing users, we'd like to allow folks to keep using their existing instrumentation and have it report to an OpenTelemetry backend. The shim wraps opentelem span types and implements the OpenTracing API.

Open Questions

I don't have a good answer here so I'd like to ask for some help here.

Baggage: What do we do with it? We can add a field to the SpanContextShim to hold baggage and propagate it in band and attach baggage to the span as attributes, however we can't exactly propagate across the wire. Additionally, @rochdev mentioned a while ago that datadog attaches vendor specific metadata to their OT context objects. How do we represent that here?
Binary Format: This is more about the limits of my knowledge of typescript type assertions and typed arrays. OpenTracing defines the carrier for binary format as an [Array-like object](https://github.com/opentracing/opentracing-javascript/blob/master/src/constants.ts#L5, but I guess how do we go from that to a Uint8Array? Would love some help with this.

TODOs

inject/extract for binary format.
comprehensive testing

packages/opentelemetry-shim-opentracing/src/shim.ts

mayurkale22

Great work! Added a few comments, will review tests in the second round.

mayurkale22 · 2019-08-26T18:57:31Z

packages/opentelemetry-shim-opentracing/README.md

@@ -0,0 +1,11 @@
+# OpenTracing shim
+[![Gitter chat][gitter-image]][gitter-url]


All the links are missing...

You should include these reference-style links at the bottom.

[gitter-image]: https://badges.gitter.im/open-telemetry/opentelemetry-js.svg [gitter-url]: https://gitter.im/open-telemetry/opentelemetry-node?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge [license-url]: https://github.com/open-telemetry/opentelemetry-js/blob/master/LICENSE [license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat [dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-shim-opentracing [dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-shim-opentracing [devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/dev-status.svg?path=packages/opentelemetry-shim-opentracing [devDependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-shim-opentracing&type=dev

yeah will add them shortly, finally have the build working :)

Looks like this is still missing.

packages/opentelemetry-shim-opentracing/src/shim.ts

mayurkale22 · 2019-08-26T19:34:59Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+  }
+
+  setTag(key: string, value: unknown): this {
+    this._span.setAttribute(key, value);


If key === opentracing.Tags.ERROR, we should report them as this._span.setStatus (CanonicalCode.OK or CanonicalCode.UNKNOWN). Also, If key === opentracing.Tags.SPAN_KIND, we should ignore that tag, see open-telemetry/opentelemetry-java#42.

I don't think we should ignore tags since that might break existing instrumentation and dashboards if the user relies on span.kind. For example, within LightStep you can define an arbitrary search and collect traces+latency metrics for that search. A common example is operation_name:my-rpc-call tag:span.kind=client and operation_name:my-rpc-call tag:span.kind=server to differentiate latency between clients and servers.

packages/opentelemetry-shim-opentracing/test/Shim.test.ts

packages/opentelemetry-shim-opentracing/package.json

mayurkale22 · 2019-08-27T16:46:04Z

Strangely, CircleCI jobs are not running for this PR. Any idea?

… bg/ot-shim

bg451 · 2019-08-27T22:02:41Z

Oh interesting... so when I created a PR for this in my forked repo, the circleCI jobs kicked off.

mayurkale22 · 2019-08-27T22:32:24Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+
+/**
+ * TracerShim wraps a {@link types.Tracer} and implements the
+ * OpenTracing span context API.


typo: s/span context API/Tracer API?

Any update here?

packages/opentelemetry-shim-opentracing/src/shim.ts

mayurkale22 · 2019-08-27T22:37:37Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+  }
+
+  getBaggageItem(key: string): string | undefined {
+    // TODO: should this go into the context?


For now, return undefined?

Could you please issues on baggage and binaryFormat (mentioned in the Open questions)? Probably we can merge this one and handle these issues separately.

packages/opentelemetry-shim-opentracing/src/shim.ts

draffensperger · 2019-08-28T02:27:01Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+   * @param finishTime An optional timestamp to explicitly set the span's end time.
+   */
+  finish(finishTime?: number): void {
+    this._span.end(finishTime);


If I recall correctly, OpenTracing uses epoch times, but we are using performance.now timestamps in OpenTelemetry for higher accuracy. Do we need to reverse translate this?

Alternatively, we could also make OpenTelemetry accept either type of timestamp - you can deterministically tell what type of timestamp it is based on its size (this is true because no Node processes or browser tabs were open in 1970...)

Per conversation in the SIG, we will make the API accept either peformance.now timestamps or epoch timestamps so that this code can be left as-is

mayurkale22

LGTM

draffensperger

LGTM pending final comment about adding a tslint.json based on the base one and the tslint-microsoft-contrib package (that change happened since this PR was started ...)

draffensperger · 2019-09-17T12:49:44Z

packages/opentelemetry-shim-opentracing/package.json

+    "access": "public"
+  },
+  "devDependencies": {
+    "@types/mocha": "^5.2.7",


Could you add a tslint.json file that matches https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/tslint.json ? And then also add the tslint-microsoft-contrib package? That will enable this package to pick up new TSLint goodies such as the license header checks, and anything else we add in later

also please add "tslint-consistent-codestyle":"^1.15.1",

mayurkale22 · 2019-09-18T16:46:52Z

@OlivierAlbertini @danielkhan @vmarchaud @hekike @rochdev ping, for reviews.

danielkhan

LGTM but I'm no OpenTracing expert. I think it's still safe to go ahead and try some end-to-end tests to discover possible issues.

bg451 · 2019-09-18T23:40:52Z

@mayurkale22 I think we have enough approvals to merge

packages/opentelemetry-shim-opentracing/LICENSE

packages/opentelemetry-shim-opentracing/package.json

OlivierAlbertini · 2019-09-18T23:46:01Z

packages/opentelemetry-shim-opentracing/package.json

+    "access": "public"
+  },
+  "devDependencies": {
+    "@types/mocha": "^5.2.7",


also please add "tslint-consistent-codestyle":"^1.15.1",

OlivierAlbertini · 2019-09-18T23:48:36Z

packages/opentelemetry-shim-opentracing/tslint.json

@@ -0,0 +1,4 @@
+{


should be

{ "rulesDirectory": ["node_modules/tslint-microsoft-contrib"], "extends": ["../../tslint.base.js", "./node_modules/tslint-consistent-codestyle"] } ```

… bg/ot-shim

OlivierAlbertini · 2019-09-18T23:56:53Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+      case opentracing.FORMAT_BINARY:
+        // @todo: Implement binary format
+        return;
+      default:


default should have logger.warning(${format} is unsupported) or never (https://basarat.gitbooks.io/typescript/docs/types/never.html)

went with logger

OlivierAlbertini · 2019-09-18T23:58:59Z

packages/opentelemetry-shim-opentracing/src/shim.ts

+    switch (format) {
+      // tslint:disable-next-line:no-switch-case-fall-through
+      case opentracing.FORMAT_HTTP_HEADERS:
+      case opentracing.FORMAT_TEXT_MAP:


I'm not sure that opentracing.FORMAT_HTTP_HEADERS is the same as opentracing.FORMAT_TEXT_MAP

since opentelemetry doesn't support a text map format, http is the closest we get. fwiw both http and text have the same carrier type in opentracing and http is a stricter subset of text map so I think they're basically equivalent, plus it's going to the same inject/extract implementation

OlivierAlbertini

LGTM. added few comments. Good job!

mayurkale22 · 2019-09-19T19:54:15Z

@bg451 Once Olivier's comments fixed, feel free to merge 👍

Signed-off-by: Naseem <naseem@transit.app>

Brandon Gonzalez added 2 commits August 23, 2019 15:34

checkpoint

0a9cfd6

add test for links and attributes

f9bf53e

bg451 force-pushed the bg/ot-shim branch from b88db5a to f9bf53e Compare August 23, 2019 19:35

Brandon Gonzalez added 5 commits August 23, 2019 15:46

lint

445b944

comment out binary inject/extract code

6ce228e

use setAttributes and addLinks

ce77e0e

Merge branch 'master' into bg/ot-shim

4018a3f

Adds test for span.finish()

f299292

bg451 commented Aug 26, 2019

View reviewed changes

packages/opentelemetry-shim-opentracing/src/shim.ts Outdated Show resolved Hide resolved

remove console.log

dc00afb

bg451 marked this pull request as ready for review August 26, 2019 18:09

bg451 requested review from danielkhan, iredelmeier, mayurkale22 and rochdev as code owners August 26, 2019 18:09

mayurkale22 reviewed Aug 26, 2019

View reviewed changes

Merge branch 'master' into bg/ot-shim

9018c28

mayurkale22 reviewed Aug 27, 2019

View reviewed changes

packages/opentelemetry-shim-opentracing/package.json Show resolved Hide resolved

bg451 changed the title ~~Add OpenTracing shim~~ feat: Add OpenTracing shim Aug 27, 2019

Brandon Gonzalez added 4 commits August 27, 2019 17:30

respond to comments round 1

9a6149c

Merge branch 'bg/ot-shim' of github.com:bg451/opentelemetry-node into…

b468c50

… bg/ot-shim

adds missing return

c6641d3

adds codecov

df32363

mayurkale22 reviewed Aug 27, 2019

View reviewed changes

draffensperger reviewed Aug 28, 2019

View reviewed changes

Brandon Gonzalez added 2 commits August 28, 2019 17:38

fix compile errors checkpoint

12fbfd7

more compile error fixing

d2fc80f

bg451 requested review from OlivierAlbertini, hekike and vmarchaud September 5, 2019 16:42

respond to comments

a46687c

mayurkale22 approved these changes Sep 16, 2019

View reviewed changes

draffensperger approved these changes Sep 17, 2019

View reviewed changes

Brandon Gonzalez added 2 commits September 17, 2019 12:11

Merge branch 'master' into bg/ot-shim

b0614ba

Adds tslint.json

20ba986

danielkhan approved these changes Sep 18, 2019

View reviewed changes

Merge branch 'master' into bg/ot-shim

38eaa9e

OlivierAlbertini reviewed Sep 18, 2019

View reviewed changes

Brandon Gonzalez added 4 commits September 18, 2019 19:53

Update license

8c278d7

respodn to comments

0c9b357

Merge branch 'master' into bg/ot-shim

5d429f7

Merge branch 'bg/ot-shim' of github.com:bg451/opentelemetry-node into…

3245eca

… bg/ot-shim

OlivierAlbertini reviewed Sep 18, 2019

View reviewed changes

OlivierAlbertini approved these changes Sep 19, 2019

View reviewed changes

Brandon Gonzalez added 2 commits September 19, 2019 12:03

ftb pt3

2abbf93

ftb

0f7c35a

draffensperger approved these changes Sep 19, 2019

View reviewed changes

Brandon Gonzalez and others added 3 commits September 20, 2019 15:28

Merge branch 'master' into bg/ot-shim

190ee52

adds logger

8790b2d

Merge branch 'master' into bg/ot-shim

7932a62

bg451 merged commit ede7886 into open-telemetry:master Sep 20, 2019

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

docs: update dns status and add hapi koa (open-telemetry#212)

6ea109a

Signed-off-by: Naseem <naseem@transit.app>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add OpenTracing shim #212

feat: Add OpenTracing shim #212

bg451 commented Aug 22, 2019 •

edited

Loading

mayurkale22 left a comment

mayurkale22 Aug 26, 2019

mayurkale22 Aug 29, 2019

bg451 Aug 29, 2019

mayurkale22 Sep 16, 2019

mayurkale22 Aug 26, 2019

bg451 Aug 27, 2019

mayurkale22 commented Aug 27, 2019

bg451 commented Aug 27, 2019

mayurkale22 Aug 27, 2019

mayurkale22 Sep 16, 2019

mayurkale22 Aug 27, 2019

mayurkale22 Sep 16, 2019

draffensperger Aug 28, 2019

draffensperger Aug 31, 2019

mayurkale22 left a comment

draffensperger left a comment

draffensperger Sep 17, 2019

OlivierAlbertini Sep 18, 2019

mayurkale22 commented Sep 18, 2019

danielkhan left a comment

bg451 commented Sep 18, 2019

OlivierAlbertini Sep 18, 2019

OlivierAlbertini Sep 18, 2019

OlivierAlbertini Sep 18, 2019 •

edited

Loading

bg451 Sep 20, 2019

OlivierAlbertini Sep 18, 2019

bg451 Sep 19, 2019

OlivierAlbertini left a comment

mayurkale22 commented Sep 19, 2019

		@@ -0,0 +1,11 @@
		# OpenTracing shim
		[![Gitter chat][gitter-image]][gitter-url]

feat: Add OpenTracing shim #212

feat: Add OpenTracing shim #212

Conversation

bg451 commented Aug 22, 2019 • edited Loading

Open Questions

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

Choose a reason for hiding this comment

mayurkale22 commented Aug 27, 2019

bg451 commented Aug 27, 2019

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

mayurkale22 left a comment

Choose a reason for hiding this comment

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Sep 18, 2019

danielkhan left a comment

Choose a reason for hiding this comment

bg451 commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini Sep 18, 2019 • edited Loading

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

OlivierAlbertini left a comment

Choose a reason for hiding this comment

mayurkale22 commented Sep 19, 2019

bg451 commented Aug 22, 2019 •

edited

Loading

OlivierAlbertini Sep 18, 2019 •

edited

Loading