feat: Add Envoy Propagator by ishg · Pull Request #264 · lightstep/lightstep-tracer-javascript · GitHub

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

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.

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)

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

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.

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.

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?

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

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?

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

why let, instead of const ?

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

                       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,

+                      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;

                       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,

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

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


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

		const BINARY_PROTO = {

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

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

		@@ -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) {

                           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);

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

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

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

feat: Add Envoy Propagator by ishg · Pull Request #264 · lightstep/lightstep-tracer-javascript · GitHub

Uh oh!

feat: Add Envoy Propagator - #264

feat: Add Envoy Propagator#264ishg wants to merge 5 commits intomasterlightstep/lightstep-tracer-javascript:masterfrom ishmeet/envoy-propagatorlightstep/lightstep-tracer-javascript:ishmeet/envoy-propagatorCopy head branch name to clipboard

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!

4 participants

feat: Add Envoy Propagator#264
ishg wants to merge 5 commits into
masterfrom
ishmeet/envoy-propagator