feat: Add Envoy Propagator #264

ishg · 2021-03-11T19:55:04Z

Adding support for Envoy Propagator, i.e. injecting Span context as a binary string under a single x-ot-span-context header. This is required for applications making requests to services behind Istio. Istio currently uses the envoy propagator which defaults to looking for this header as an incoming value to decode existence of trace.

cc: @austinlparker

ishg · 2021-03-11T20:20:38Z

src/imp/propagator_envoy.js

+
+const CARRIER_ENVOY_HEADER_KEY = 'x-ot-span-context';
+
+const BINARY_PROTO = {


Not sure if this should be in here like this. I tried using a local file but it doesn't get generated with webpack. Any tips or recommendations for proto?

Not sure if this should be in here like this. I tried using a local file but it doesn't get generated with webpack. Any tips or recommendations for proto?

I think we should generate the lightstep.proto in the same way that we generate the collector.proto. The resulting file would end up in the src/imp/generated_proto directory. See the Makefile.

If we do that, we can probably avoid the dependency on protobufjs since we already use google-protobuf.

ishg · 2021-03-11T20:21:31Z

src/imp/tracer_imp.js

        this._propagators[this._opentracing.FORMAT_HTTP_HEADERS] = new LightStepPropagator(this);
        this._propagators[this._opentracing.FORMAT_TEXT_MAP] = new LightStepPropagator(this);
-        this._propagators[this._opentracing.FORMAT_BINARY] = new UnsupportedPropagator(this,
+        this._propagators[this._opentracing.FORMAT_BINARY] = new LightStepPropagator(this,


Does it make sense to have the LightstepPropagator now have a default for FORMAT_BINARY?

It looks like go uses the BinaryPropagator by default: https://github.com/lightstep/lightstep-tracer-go/blob/master/tracer.go#L180. I think that corresponds to the EnvoyPropagator from this PR.

I think this makes sense, but have we thought about how this will work with or impact the no-protobuf version of this library?

FORMAT_BINARY should correspond to the binary propagator. BinaryPropagator/EnvoyPropagator/whatever are all the same.

Theoretically this shouldn't impact the no-pb version (as FORMAT_BINARY should map to UnsupportedPropagator correctly over there)

ishg · 2021-03-11T20:22:11Z

test/suites/common/tracer.js

        });

-        it("should propagate binary carriers");
+        it('should propagate binary carriers', () => {


This is the modified test in here, the rest of the changes were just formatting based on es-lint.

mwear

I am not super familiar with the lightstep binary format, so perhaps @austinlparker can weigh in to confirm or correct some of my feedback in this review. I made a pass through this and have provided some suggestions on how I think this should be done though.

mwear · 2021-03-11T23:27:51Z

src/imp/propagator_envoy.js

+
+const CARRIER_ENVOY_HEADER_KEY = 'x-ot-span-context';
+
+const BINARY_PROTO = {


Not sure if this should be in here like this. I tried using a local file but it doesn't get generated with webpack. Any tips or recommendations for proto?

I think we should generate the lightstep.proto in the same way that we generate the collector.proto. The resulting file would end up in the src/imp/generated_proto directory. See the Makefile.

If we do that, we can probably avoid the dependency on protobufjs since we already use google-protobuf.

mwear · 2021-03-11T23:42:30Z

src/imp/propagator_envoy.js

+        let msg = binaryCarrier.create(payload);
+        let buffer = binaryCarrier.encode(msg).finish();
+        let bufferString = pb.util.base64.encode(buffer, 0, buffer.length);
+        carrier[this._envoyHeaderKey] = bufferString;


I could be mistaken, but for binary, isn't the carrier just a byte array? If so, I don't think there is a notion of a string key, with a binary value. I think it just writes bytes into the array.

it still needs a key in the headers, and you still need to base64 the output. maybe this would be one-step with google protos?

keep in mind that adding another proto will cause the tracer file size to creep up again, to your above point

Do any of the other Lightstep tracers have an EnvoyPropagator? As I look around I only see BinaryPropagator (this is true for Go and Python anyway).

C++ has one too that just uses the binary propagator to do the encoding but then puts it in the envoy key header.

I was thinking that we should probably try to do the same. Would it be possible to split to this work into a BinaryPropagator and an Envoy propagator (or instrumentation) that uses it to do the header manipulation?

mwear · 2021-03-11T23:45:02Z

src/imp/propagator_envoy.js

+        let traceGUID = null;
+        let sampled = true;
+
+        if (carrier[this._envoyHeaderKey] === undefined) {


I believe the same comment for inject about the carrier being a byte array applies here as well.

mwear · 2021-03-11T23:46:10Z

src/imp/tracer_imp.js

        this._propagators[this._opentracing.FORMAT_HTTP_HEADERS] = new LightStepPropagator(this);
        this._propagators[this._opentracing.FORMAT_TEXT_MAP] = new LightStepPropagator(this);
-        this._propagators[this._opentracing.FORMAT_BINARY] = new UnsupportedPropagator(this,
+        this._propagators[this._opentracing.FORMAT_BINARY] = new LightStepPropagator(this,


It looks like go uses the BinaryPropagator by default: https://github.com/lightstep/lightstep-tracer-go/blob/master/tracer.go#L180. I think that corresponds to the EnvoyPropagator from this PR.

I think this makes sense, but have we thought about how this will work with or impact the no-protobuf version of this library?

obecny · 2021-03-12T11:37:23Z

src/_leftpad.js

@@ -0,0 +1,11 @@
+// Underscore.js-like wrapper to left pad a string to a certain length with a character
+export default function _leftpad(str, len, ch) {


is this going to be used in node.js only ? , if so then use String.prototype.repeat instead

obecny · 2021-03-12T11:40:11Z

test/suites/common/tracer.js

-            var span = Tracer.startSpan('my_span');
-            var spanContext = span.context();
+        it('should propagate text map carriers', () => {
+            let span = Tracer.startSpan('my_span');


why let, instead of const ?

obecny · 2021-03-12T11:40:29Z

test/suites/common/tracer.js

-            var span = Tracer.startSpan('my_span');
-            var spanContext = span.context();
+        it('should propagate http header carriers', () => {
+            let span = Tracer.startSpan('my_span');


let -> const

obecny · 2021-03-12T11:40:40Z

test/suites/common/tracer.js

            Tracer._propagators[Tracer._opentracing.FORMAT_HTTP_HEADERS] = new lightstep.B3Propagator(Tracer);
-            var span = Tracer.startSpan('my_span');
-            var spanContext = span.context();
+            let span = Tracer.startSpan('my_span');


let -> const

obecny · 2021-03-12T11:40:49Z

test/suites/common/tracer.js

            expect(carrier['ot-baggage-creditcard']).to.equal('visa');

-            var extractedContext = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, carrier);
+            let extractedContext = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, carrier);


let -> const

obecny · 2021-03-12T11:41:06Z

test/suites/common/tracer.js

            };

-            var context = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers);
+            let context = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers);


let -> const

obecny · 2021-03-12T11:41:18Z

test/suites/common/tracer.js

            };

-            var context = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers);
+            let context = Tracer.extract(opentracing.FORMAT_HTTP_HEADERS, headers);


let -> const

obecny · 2021-03-12T11:41:39Z

test/suites/common/tracer.js

+        it('should propagate binary carriers', () => {
+            Tracer._propagators[Tracer._opentracing.FORMAT_BINARY] = new lightstep.EnvoyPropagator(Tracer);
+
+            let carrier = {};


let -> const

obecny · 2021-03-12T11:42:04Z

test/suites/common/tracer.js

-            var span = Tracer.startSpan('test');
+    describe('Tracer#flush', () => {
+        it('supports passing no arguments', () => {
+            let span = Tracer.startSpan('test');


let -> const

obecny · 2021-03-12T11:42:11Z

test/suites/common/tracer.js

-        it("supports passing a callback argument", function(done) {
-            var span = Tracer.startSpan('test');
+        it('supports passing a callback argument', (done) => {
+            let span = Tracer.startSpan('test');


let -> const

ishg added 3 commits March 11, 2021 11:46

Adding Envoy Propagator

42631c9

Adding tests for Envoy Propagator

a56be65

Building dist

5bf98a3

ishg requested review from mwear and obecny March 11, 2021 19:55

ishg added 2 commits March 11, 2021 12:15

Fixing test for binary prop and adding left pad to span context ids

2007b93

Removing console.log and comments

0bb5cae

ishg commented Mar 11, 2021

View reviewed changes

ishg added the help wanted label Mar 11, 2021

mwear suggested changes Mar 11, 2021

View reviewed changes

obecny reviewed Mar 12, 2021

View reviewed changes


		const CARRIER_ENVOY_HEADER_KEY = 'x-ot-span-context';

		const BINARY_PROTO = {

		@@ -0,0 +1,11 @@
		// Underscore.js-like wrapper to left pad a string to a certain length with a character
		export default function _leftpad(str, len, ch) {

feat: Add Envoy Propagator #264

Are you sure you want to change the base?

feat: Add Envoy Propagator #264

Uh oh!

Conversation

ishg commented Mar 11, 2021

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants