Add Propagation interface about zipkin-js

[asanluis]

Yeah, i'm going to try 😅😁!!!! When i have something i'll open a PR and wait the feedback. Thanks as always!!!

[asanluis]

@jcchavezs, not enough time 😞, just waiting to have a little of free time to work in this!!😉

[jcchavezs]

That is actually something desirable! This library does not have interfaces for propagation so it would be great to start one. Notice that currently, all the HTTP server instrumentation includes a function called readHeader in the recordRequest whose aim is to extract the headers from the values from the headers. I guess maybe that was because of the lack of standard headers in node? (just guessing here).

IMHO the easiest is to deprecate that readHeader and either:
a) delegate the responsibility of such thing into the tracer (like zipkin-go does) when readHeader is not passed? Ideally Tracer and Propagation should be separated like in brave or zipkin-php but this alternative does not feel too bad.
b) If including extract/inject in the tracer is not acceptable, then probably we can inject the propagation object into httpInstrumentation (still we will have an abstraction to propagation but not inside the tracer) and to the extract from there.

For client it is easier, that piece is already abstracted under addZipkinHeaders so we just need to tweak it to align with other interfaces and make it pluggable.

Are you up to give a stab on this?

Thoughts @adriancole @anuraaga

[anuraaga]

Interop is the name of the game, very happy to hear about adding support for more propagations.

Not familiar enough with the js codebase but b) feels a bit nicer if it's practical. But don't feel too strongly either way.

[jcchavezs]

@asanluis do you need any further help on this one?

[asanluis]

@jcchavezs Hi, I've not forgotten 😥 but now i have a little of free time, and i want to ask, if you guys like this approach

namespace propagation {
    interface Propagation {
      extractor<T>(readHeader: <T> (header: string) => option.IOption<T>): TraceId;
      injector<T, H>(req: T & { headers?: any }, traceId: TraceId): RequestZipkinHeaders<T, H>;
    }
    class B3Propagation implements Propagation {
      extractor<T>(readHeader: <T> (header: string) => option.IOption<T>): TraceId;
      injector<T, H>(req: T & { headers?: any }, traceId: TraceId): RequestZipkinHeaders<T, H>;
    }
  }

the extractor would be the actual _createIdFromHeaders of the HttpServer class, and the injector would be the actual Request.addZipkinHeaders.

[asanluis]

Hi guys, i finally made a different approach, i add the propagation into the tracer 😢 , not the best implementation, but i think is the best way to easily add the baggage feature. i'll wait your feedback guys, Thanks for all!! changes branch-master

[jcchavezs]

Awesome. I will check it out. ons. 15. jul. 2020, 21:15 skrev asanluis <[email protected]>:

…

@jcchavezs <https://github.com/jcchavezs> Hi, I've not forgotten 😥 but now i have a little of free time, and i want to ask, if you guys like this approach namespace propagation { interface Propagation { extractor<T>(readHeader: <T> (header: string) => option.IOption<T>): TraceId; injector(req: { headers?: any }, traceId: TraceId): ZipkinHeader; } class B3Propagation implements Propagation { extractor<T>(readHeader: <T> (header: string) => option.IOption<T>): TraceId; injector(req: { headers?: any }, traceId: TraceId): ZipkinHeader; } } the extractor would be the actual _createIdFromHeaders of the HttpServer class, and the injector would be the actual Request.addZipkinHeaders — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#496 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYARFGUFNLL4XAK2765TR3X55XANCNFSM4MOIE2BQ> .

[jcchavezs]

Hi @asanluis thanks for bringing it up. I think the approach is correct, I'd encourage you to open a PR with the changes so I can give you some feedback!

[asanluis]

I finally made the PR, i have to work in the grpc client 😞

Edit i add a comment:
https://github.com/openzipkin/zipkin-js/pull/510/files#r465894515

[asanluis]

@jcchavezs have you had time to review it?

[jcchavezs]

Hi @asanluis I am sorry I did not come back to you earlier. Crazy days and a job switch in the middle. I am looking at it today and in general will have more time to spend on this. Stay tuned.

[asanluis]

Hi @jcchavezs thanks for the feedback 😄, I like the Java approach of Getter and Setter in the injectors/extractor methods i added here, now im working in the implementation.

What about if i change the implementation and decouple the propagation from the tracer, and maybe put it in the instrumentation?