Add tracing spans for input gate hold

[shrima-cf]

No description provided.

[justin-mp]

I still don't know what's preventing us from hoisting the creation of span to a layer higher to where we actually wait for the lock. This seems to have all of the problems of the previous approach.

[shrima-cf]

I still don't know what's preventing us from hoisting the creation of span to a layer higher to where we actually wait for the lock. This seems to have all of the problems of the previous approach.

I thought doing it in this place felt more easy to understand. How do you say this has all the problems of the previous approach? The span is now stored in the IncomingRequest object, which is one per request. So we will have separate tracing per request as opposed to previously I had the span in the gate (which is wrong because gate is shared across requests)

[a-robinson]

I still don't know what's preventing us from hoisting the creation of span to a layer higher to where we actually wait for the lock. This seems to have all of the problems of the previous approach.

To be fair, this PR doesn't even touch on waiting for the lock, it's merely about holding the lock. But I agree that it's weird to stick tracing into hooks rather than into the InputGate::Lock itself.

I can't totally tell -- was it done this way to get around the issue mentioned in #5563 (comment) of there not being an IoContext available when the InputGate lock is being taken? If so I can understand why we'd go this way, otherwise I'd be interested in understanding the reasoning (and ideally having a comment about it, since this looks a little odd).

[a-robinson]

I can't totally tell -- was it done this way to get around the issue mentioned in #5563 (comment) of there not being an IoContext available when the InputGate lock is being taken? If so I can understand why we'd go this way, otherwise I'd be interested in understanding the reasoning (and ideally having a comment about it, since this looks a little odd).

Alternatively, if we're just needing to work around the lack of a current IoContext, we could have the hook return the span object such that the lock can own it? The benefits in my mind would be:

• Taking advantage of RAII rather than needing to ensure we reliably call the method to close the span
• Not cluttering IoContext_IncomingRequest with more feature-specific details from other parts of the code base
• (Hopefully) Making it easier to set tags on the span, e.g. if an input gate critical section fails. Althouhg this is more debatable, since we likely want to set an error tag on all incoming DO request spans if they're failing due to the DO breaking, and if we're already doing that then also doing it on the input_gate_hold span is less important

[shrima-cf]

I can't totally tell -- was it done this way to get around the issue mentioned in #5563 (comment) of there not being an IoContext available when the InputGate lock is being taken? If so I can understand why we'd go this way, otherwise I'd be interested in understanding the reasoning (and ideally having a comment about it, since this looks a little odd).

Alternatively, if we're just needing to work around the lack of a current IoContext, we could have the hook return the span object such that the lock can own it? The benefits in my mind would be:

• Taking advantage of RAII rather than needing to ensure we reliably call the method to close the span
• Not cluttering IoContext_IncomingRequest with more feature-specific details from other parts of the code base
• (Hopefully) Making it easier to set tags on the span, e.g. if an input gate critical section fails. Althouhg this is more debatable, since we likely want to set an error tag on all incoming DO request spans if they're failing due to the DO breaking, and if we're already doing that then also doing it on the input_gate_hold span is less important

Yes, it all started with the issue that we don't have IoContext set when we acquire the lock.

I initially started with adding the span on the Lock, but the problem is, I need to get the parent span from IoContext, this resulted in a circular dependency since io context depends on io-gate. Lock is inside Io-gate.

Regarding the hook returning the span object so the lock can hold it - this is something I had in my mind, but did not go that direction since I wanted to tie the span start and end to the lock acquired/release method.

I am currently trying to figure out how to make this work for Wait span, so far, it looks like this approach will not work there since each waiter needs to have its own span and I cannot piggy back on the getCurrentRequest for that. So maybe I will have to go the way of hooks returning a span that can be held in the Lock/Waiter.

[jasnell]

Rebase from main to clear the wpt-related failure in CI.

[shrima-cf]

I can't totally tell -- was it done this way to get around the issue mentioned in #5563 (comment) of there not being an IoContext available when the InputGate lock is being taken? If so I can understand why we'd go this way, otherwise I'd be interested in understanding the reasoning (and ideally having a comment about it, since this looks a little odd).

Alternatively, if we're just needing to work around the lack of a current IoContext, we could have the hook return the span object such that the lock can own it? The benefits in my mind would be:

• Taking advantage of RAII rather than needing to ensure we reliably call the method to close the span
• Not cluttering IoContext_IncomingRequest with more feature-specific details from other parts of the code base
• (Hopefully) Making it easier to set tags on the span, e.g. if an input gate critical section fails. Althouhg this is more debatable, since we likely want to set an error tag on all incoming DO request spans if they're failing due to the DO breaking, and if we're already doing that then also doing it on the input_gate_hold span is less important

Yes, it all started with the issue that we don't have IoContext set when we acquire the lock.

I initially started with adding the span on the Lock, but the problem is, I need to get the parent span from IoContext, this resulted in a circular dependency since io context depends on io-gate. Lock is inside Io-gate.

Regarding the hook returning the span object so the lock can hold it - this is something I had in my mind, but did not go that direction since I wanted to tie the span start and end to the lock acquired/release method.

I am currently trying to figure out how to make this work for Wait span, so far, it looks like this approach will not work there since each waiter needs to have its own span and I cannot piggy back on the getCurrentRequest for that. So maybe I will have to go the way of hooks returning a span that can be held in the Lock/Waiter.

I discussed with Justin a bit about this, I am going to try and see if there is any other way to get the span passed into the wait method in InputGate, that will help solve the problem of both hold span and wait span. Last time I tried to do this, I had issues because the context was not available in the run method where the wait was called, explained in #5563 (comment) , will see if Claude can find some other way

[a-robinson]

Thanks for explaining! Given your explanation I think this approach is bearable, but yeah I think that passing the span into the wait() call would be preferable. And it looks doable! The call here is in an IoContext, the call here has an IoContext available, and IIUC the one call in our internal code that doesn't have an IoContext does have a SpanParent that you could pass in.