add propagation interface #510
Conversation
// TODO if is the correct approach - Delete RequestZipkinHeader - Refactor kafkajs and expressjs instrumentation - Update README, add example usage of a custom propagation - Add AWS propagation module
// TODO if is the correct approach - Delete RequestZipkinHeader - Refactor kafkajs and expressjs instrumentation - Update README, add example usage of a custom propagation - Add AWS propagation module
// TODO if is the correct approach - Delete RequestZipkinHeader - Refactor kafkajs and expressjs instrumentation - Update README, add example usage of a custom propagation - Add AWS propagation module
| }) { | ||
| this.log = log; | ||
| this.recorder = recorder; | ||
| this.sampler = sampler; | ||
| this.traceId128Bit = traceId128Bit; | ||
| this.supportsJoin = supportsJoin; | ||
| this._propagation = propagation; |
There was a problem hiding this comment.
Don't know if a better approach is delete the extract and injector methods and only add a get propagation, this simplify the grpc-client implementation, thats need to know the propagation headers....
There was a problem hiding this comment.
I would go for that instead.
| @@ -18,7 +18,7 @@ class ExpressHttpProxyInstrumentation { | |||
| const clientTraceId = this.tracer.createChildId(); | |||
| this.tracer.setId(clientTraceId); | |||
|
|
|||
| const proxyReqWithZipkinHeaders = Request.addZipkinHeaders(proxyReq, clientTraceId); | |||
| const proxyReqWithZipkinHeaders = this.tracer.injector(proxyReq); | |||
There was a problem hiding this comment.
I'd rather pass the clientTraceId explicitly instead of having the injector to know about the scope.
| @@ -61,6 +62,11 @@ declare namespace zipkin { | |||
| recordLocalAddr(inetAddress: InetAddress): void; | |||
| recordBinary(key: string, value: boolean | string | number): void; | |||
| writeIdToConsole(message: any): void; | |||
| /** Extract propagation ctx from request */ | |||
| extractId(readHeader: <T> (header: string) => option.IOption<T>): void; | |||
There was a problem hiding this comment.
I think I'd prefer to have the extractId to be actually an extractor like the injector.
Also, maybe read header can be an abstract getter, not necessarily tight to http headers (e.g. in GRPC it is metadata)?
| /** Extract propagation ctx from request */ | ||
| extractId(readHeader: <T> (header: string) => option.IOption<T>): void; | ||
| /** Injector propagation ctx from request */ | ||
| injector(request: any): object; |
There was a problem hiding this comment.
injector can be for messaging too so I think it is better that we pass something like readHeader and call it setter and this return an object that performs the injection, does that make sense?
packages/zipkin/index.d.ts
Outdated
| @@ -368,6 +367,16 @@ declare namespace zipkin { | |||
| recordError(traceId: TraceId, error: Error): void; | |||
| } | |||
| } | |||
| namespace propagation { | |||
| interface Propagation { | |||
| extractor<T>(tracer: Tracer, readHeader: <T> (header: string) => option.IOption<T>): TraceId; | |||
There was a problem hiding this comment.
If I understand correctly, we pass the tracer to the extractor? is this really needed?
There was a problem hiding this comment.
In new tracing models we have propagation decoupled from tracer, I think we should follow similar pattern here.
There was a problem hiding this comment.
agree we should not couple these
| @@ -16,7 +15,7 @@ export { default as ConsoleRecorder } from './console-recorder'; | |||
| export { default as ExplicitContext } from './explicit-context'; | |||
|
|
|||
| export { default as Instrumentation } from './instrumentation'; | |||
| export { default as Request } from './request'; | |||
| export { default as B3Propagation } from './propagation/b3propagation'; | |||
| @@ -74,7 +27,7 @@ class HttpServerInstrumentation { | |||
| } | |||
|
|
|||
| recordRequest(method, requestUrl, readHeader) { | |||
| this._createIdFromHeaders(readHeader).ifPresent(id => this.tracer.setId(id)); | |||
| this.tracer.extractId(readHeader); | |||
There was a problem hiding this comment.
I am really happy this becomes so simple now.
| headers[this.headers.Sampled] = sampled ? '1' : '0'; | ||
| }); | ||
|
|
||
| if (traceId.isDebug()) { |
There was a problem hiding this comment.
In here, if `traceId.isDebug()) then you don't need to pass sampling.
|
This is great work @asanluis I left you some comments cc @adriancole |
|
I will come back to it this week. |
TODO if is the correct approach