How can I add tags per request?

[jeffthompson1971]

I have this working, but i need to pass in a couple specific tags like username etc so i know who the trace is for. I don't see a way to do that. I am referring to the express implementation. i will have custom fields added to the request object...

was thinking maybe I could do a pull request that looks for a 'tags' object on the request object, and if that's there it adds to the span? if not then nobody breaks and it's backwards compatible?

[codefromthecrypt]

are these tags http headers?

[codefromthecrypt]

or are you looking for a way for us to propagate something beyond the traceId?

[jeffthompson1971]

yes.. this are like username and an organization identifier.. i add them to every trace on client-side with the axios implementation. but some of our node services will not have their corresponding web apps instrumented... but still want to get those tags in the express traces

[jeffthompson1971]

i can fork this and try some stuff out?

[codefromthecrypt]

consider stuffing it in the traceID object because you aren't guaranteed to not have any spans between http in and http out. that's my advice.

in the java side, we have this thing called "extra" which is basically a field inside TraceId to store stuff. it is intentionally opaque list to allow multiple plugins to not clobber eachother. one plugin is called "extra fields" which allows you to propagate something from headers to the other side.

[jeffthompson1971]

hmm.. so i already just use the defaultTags so that the fields are stand-alone and can be queried. if I have stuff in a traceID object how could i query for all spans from user thompjg@business.com for example?
this is what i have.
`const tracer = new Tracer({
sampler: theSampler,
ctxImpl: new CLSContext('zipkin'),
recorder: new BatchRecorder({
logger: new HttpLogger({
endpoint: TRACER_ENDPOINT,
jsonEncoder: jsonEncoder.JSON_V2,
}),
}),
localServiceName: SERVICE_NAME,
defaultTags: { component: 'TracerMiddleware', project: 'aip-core-ui' },
});

export const TracerMiddleware = zipkinMiddleware.expressMiddleware({ tracer });`

[codefromthecrypt]

I maybe guessed wrong on what you need. you are not asking to propagate tags, rather just add request-specific tags (ex parsing). I think this would be a change to httpServerInstrumentation, to add a parsing function which will then sink the tags into the span.

[jcchavezs]

My only concern about passing a function is that passing the arguments to such functions might be per request and not globals. If such information comes from the request (like in headers if you translate the jwt into actual info by a proxy) then I would suggest something like `requestReader` similar to `responseHeader` in #432. Definitively something that should end up in httpInstrumentation.

[jeffthompson1971]

Yes definately what I need are tags that are per-request and NOT global. basically it's simple - we just want to know that user and tenantID (the customer identifier in our system)...

So what I would prefer to do is have some upstream express middlware that sets the name=value pairs i need ONTO the request object (in header?) and then we can have zipkin (where I need help with) look for a known header element and if it's there it adds the tags to the span.
Why not have a header key like 'trace-tags' and that value can be a JSON string like
{user: 'jeffthompson@gmail.com', tenantId: '23423432432432'}

then it's very simple and innocuous change in zipkin-js somewhere that just looks for this header and then just do this right before sending the span:
a) convert the JSON string element in the header 'trace-tags' to a JSON object
b) iterate over the keys and log an annotation on the span for each one

thoughts?

[codefromthecrypt]

I don't think we want to get opinionated on the representation in headers, but I don't think you'd need this layer to do that anyway. If I understand correctly, you aren't needing to propagate anything, just parse the headers into the span. In this case a request-based parser function seems to be the best solution. While this would already be a feature of a future "v2" api as we already do this in later tracing design, it wouldn't be hard to make a tactical one here. Let's just try to get the syntax right. I think the last thing I asked was.. which parts of the request are needed? In this case you are only looking at the headers. If you think about parsing in general, we at least also look at the method, status and path. So, I think that instead of adding a whack-a-mole which only covers headers (then having to break that api later), it is best to actually enclose all the parsing logic, with a function of the request (or response) and also a span object (which accepts tag or name). The fact that the implementation of this object would iterate over tags and add annotations internally is fine because it allows the design to be forwards compatible to a v2 api which doens't use binary annotations anymore anyway. is this too abstract??

[jeffthompson1971]

Thanks for the help Adrian. I'm trying to wrap my head completely around it. I'm kinda new - like for example I'm not even clear what the distinction is between a tag and an annotation ;) .

It is a bit abstract - can you like jot down a little psuedo-code for how this API will be extended etc.? That would help me better understand what you are proposing. are you saying you will add a new parameter that will be a function that does the parsing of the request object that I need - be it header or other bits? can you spec that out a little?

[codefromthecrypt]

@jeffthompson1971 sorry.. yeah we are years overdue getting this api overhauled. One main reason is that annotation jargon is so confusing. In real life people almost never need annotations in the v2 model. tags are for lookup etc https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml#L307

In the java library, we expose a parser object people can override, only used to parse name and tags ex. https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpParser.java#L59-L64

you can see we do similar in zipkin-js now, just we don't expose a parsing function

[poorejc]

Hey folks!

Great project. I want to add myself as an interested party in this feature. I'm willing to help, too.

Our use case: I represent Apache Flagon--we do systemic thin-client instrumentation for user behavioral analysis (https://github.com/apache/incubator-flagon-useralejs). We'd like to be able to correlate zipkin traces with user behavior at both the event and workflow model level. This means that at request, we need to parse and write data to traces to allow for cross-references with up and downstream behavioral logs. My thinking is that we need a few things:

1. Germane to this ticket: get traceID and maybe spanID from a trace at request, and add that as a key value pair into our logs. Ideally, we are able to add a UID or IRI from our logs into either Zipkin annotations or tags (probably tags makes most sense). Looking at this thread, @adriancole seems to say that "we do similar in zipkin-js now, just we don't expose a parsing function"... does that mean that zipkin-js traces can be parsed, the function just isn't exposed through the package, or that a callback function needs to be written to parse and then expose. Give us a little more scaffolding (point us to relevant src), and we're willing to push a PR on this.

2. An alternative we've explored is to add a customLog function that packages our logs in zipkin format to push directly to the zipkin API, in which case we might considered trying to use tracer.scope (referenced in How to get the traceId, parentId, spanId of current request to use outside the tracer? #128) to coerce both our logs and zipkin traces to have the same traceID. Do you have any docs on tracer.scope you can point us to? We're having trouble finding it. Also, another issue we're having with this method is that wrapping fetch calls, for example, with zipkin-instrumentation-fetch appears to affect event handling--namely this doesn't work as it would for a standard fetch call:

self.addEventListener('fetch', function () { console.log("fetch"); });

This makes triggering other logs off the same fetch call tricky. Any comment you have here would be really helpful. Fully willing to admit that it could be a scope issue, I haven't experimented a ton with this, but am using a modified example of the zipkin-js 'web' example you provide.

[codefromthecrypt]

what I meant to say is we don't currently have at initialization time of http client or server instrumentation, a function of httprequest/response to parse that into tags. instead the parsing details are constant (not terrible to do this).

concretely the constructor to https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/instrumentation/httpClient.js#L10

could take a function for parsing tags of request/response (probably that function can also handle the span name)

[jcchavezs]

So two things to clarify here. First of all the original issue was to add tags based on a request on the server. This is, when a server receives the request, having a user defined function that parses that request, adds the desired tags and then continues with normal flow.

What @adriancole is pointing out is to do this in the client side (i.e. your service adding the tags to the span that represents the client request) which is a bit simpler. Main difference is that client recordRequest receives the request as first parameter: https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/instrumentation/httpClient.js#L20 while server recordRequest receives details of the request: https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/instrumentation/httpServer.js#L76.

From the details of the ticket I feel like you are looking for this to happen on server side but I might be wrong.

In client side, it is as easy as pass an argument in the constructor that is called in the recordRequest and there one can call this.tracer.recordBinary('my_tag', 'something_from_the_request'). A consistent design we have been implementing is the httpTracing component. Something like:

httpTracing = {
requestParser(request): void;
responseParser(response): void;
}

In client side it is straightforward because we have access to the request.

In server side, we don't

recordRequest(method, requestUrl, readHeader) {

So I'd say we do some sort of polymorfism where if the first argument is a request we use it and if it is a string we assume it is the method.

In any case I am happy to help with reviews and further discussions.

[jcchavezs]

@poorejc anything else I can help with to get this started?