Create and assign an InvocationSpanContext for every invocation #3145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fhanau
reviewed
Nov 21, 2024
The thoughts above seem reasonable, I don't have much to add. Intuitively, having the InvocationSpanContext be owned by RequestObserver in some capacity feels more sensible/easier to support, but I could also see it work with IncomingRequest. Having it in/around RequestObserver would be more in line with the current model, but I guess we're making big changes anyway. |
fhanau
approved these changes
Nov 22, 2024
|
Ok, well, we can keep it here for now and can always move it later if we discover it makes more sense in |
jasnell
force-pushed
the
jsnell/invocationspancontext-for-every-invocation
branch
from
ff8a831 to
47560cf
Compare
jasnell
deleted the
jsnell/invocationspancontext-for-every-invocation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@fhanau ... please consider this one carefully. What I'm not sure about is whether the
InvocationSpanContextshould be held by theIncomingRequestor something internal to theRequestObserver.Eventually I'd like to abstract out the
WorkerTracerhere and actually hide that inside theRequestObserverand put it entirely behindSpanBuilderbut that'll come later. Accordingly, however, I don't want to putInvocationSpanContextinside theWorkerTracer.The other thing to keep in mind is that we'll need to be able to propagate the
InvocationSpanContextacross subrequests of various kinds so we'll need easy access to it to place it into things like the control header.How is this expected to be used? Good question! When streaming tail workers is fully operational, the invocation span context will be recorded with each mark event. Every child span created during the invocation of a worker will become a child off this root and should have its own
kj::Rc<InvocationSpanContext>and there should always be a "current invocation span context" at any given time during an invocation that is propagated via the async context just like the current SpanParent. The fact that the user SpanBuilder would need to have access to this suggests to me that maybe the rootInvocationSpanContextshould be owned by theRequestObserverbut I'm not entirely sure.