Allow using the same value for RequestId and Brave span ID

[trustin]

At the Armeria workshop Tokyo, we talked about using a certain request header value (or a RequestContext property) as a RequestId, which will be useful when a user is using distributed tracing already.

We could provide a RequestIdGenerator that looks for a certain decorator that implements a certain interface (e.g. BraveClient implements RequestIdGenerator) in the decorator chain and delegate the ID generation to it. This assumes that Brave allows us to 1) access its ID generator and 2) specify a previously generated ID to a span instead of always auto-generating.

Would this make sense? /cc @anuraaga @adriancole

[trustin]

For the client side, we could provide a RequestIdGenerator that looks for a certain decorator that implements a certain interface (e.g. BraveClient implements RequestIdGenerator) in the decorator chain and delegate the ID generation to it.

This assumes that Brave allows us to 1) access its ID generator and 2) specify a previously generated ID to a span instead of always auto-generating.

[anuraaga]

Is this the workshop I went to a while ago? Can't remember the RequestId topic but I think both server / client would take the same approach. Even for a server, the "current request" ID is not a request header, the parent ID would be though. Request ID = span ID for the current request is still generated in our decorators.

[trustin]

Is this the workshop I went to a while ago?

Yeah.

I think both server / client would take the same approach. Even for a server, the "current request" ID is not a request header, the parent ID would be though.

Aye, that was a mistake. Let me update the original post. 😅

[codefromthecrypt]

hmm request IDs are usually request (aka trace) scoped right? so it would not be about parent ID rather incoming trace ID except edge case where something intentionally broke it.

Main thing might be ordering.. ex if you have the trace context prior then you can simply access the trace ID there and copy it.

aside, but request-ids usually are not the same as trace ID and exposing them has some impact, though most may not care. openzipkin/b3-propagation#4

there are some interesting notes about messaging and request ids that aren't 100% same here, but interesting background as it discusses ids that are not uniform for the entire "request" https://github.com/openzipkin/brave/blob/master/instrumentation/messaging/RATIONALE.md#message-id

[minwoox]

hmm request IDs are usually request (aka trace) scoped right? so it would not be about parent ID rather incoming trace ID except edge case where something intentionally broke it.

I think we are going to use span ID as the RequestId so that ServiceRequestContext and ClientRequestContext have different request Ids.

If we introduce RequestIdGenerator and BraveService implements it, BraveService will provide a RequestId a little earlier which is when the ServiceRequestContext is created.
Then, the RequestId of the ServiceRequestContext will be set as the span ID in BraveService.serve(ctx, req) when Span is created. (Although I have no idea how to set the span ID 😆 )

[trustin]

Changed the title of the issue since the original one was incorrect.

[codefromthecrypt]

I feel calling something request ID in this way is confusingly different than how others say request ID in HTTP and RPC contexts, so don't really think this feature should exist.

[trustin]

Don't an Armeria RequestContext and a Brave span always match? Are you suggesting to renaming RequestId to something less confusing, such as RequestContextId? Just in case it wasn't clear, this feature will not be the default behavior - only for those who opt in, e.g. specify BraveRequestIdGenerator explicitly.

[trustin]

(I think I confused many people in this page by starting it incorrectly. This issue is not about using trace ID as Armeria's RequestId but about using span ID. Also, Armeria's RequestId is assigned for each RequestContext, which spans for a single span in Brave's perspective.)

[codefromthecrypt]

request ID is commonly used like a trace ID, so it doesn't change as it propagates downwards. I think that the original discussion was about request headers made me concerned about making a header like "requestId: abcd" and have that vary hop to hop

[codefromthecrypt]

RequestContextId has a nice side effect that it avoids this problem of assuming this is the same as a request Id propagated in headers.. if it is in fact not :D

[codefromthecrypt]

if we are solving to have IDs match, which seems a noble goal, one way is to allow the span to allocate sooner so that it can be used for the context ID. we overwrite timestamps anyway. This avoids us having to solve pretty significant edge cases around propagation context forking (ex you can have partial incoming context and if statements around ID are easier to solve if we know even if an ID would be allocated)

[trustin]

Creating a new span currently involves non-trivial steps like creating a request adapter and updating request headers, which doesn't seem to be too much responsibility for a request ID generator. What I was thinking of was rather simple:

1. Generate some random ID compatible with Brave (or use Brave's span ID generator) early.
2. Modify Armeria's BraveClient and BraveService to use what's generated in the step 1 when creating a new span. To do this, Brave needs to allow us to specify a span ID when creating a new span, which is not possible at the moment.

[codefromthecrypt]

the apis to continue a span allow you to create a span first. it does not require you to update headers. this is simplified here into the same lines as there was no reason to do otherwise.

[codefromthecrypt]

ex on the client side there's this hook https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpClientHandler.java#L143

if you are already propagating doing the trace context earlier also allows the logs to be aligned, rather than partially so. Also, it is very odd to indicate only a span ID, much less a request-scoped ID generator. This hints at an ordering problem I think we should consider resolving instead of digging a hole