-
Notifications
You must be signed in to change notification settings - Fork 479
Add tracing spans for input and output gates hold and wait #5563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ab5df90 to
1bf2f83
Compare
1bf2f83 to
9f6f912
Compare
| // https://opensource.org/licenses/Apache-2.0 | ||
|
|
||
| #include "actor-cache.h" | ||
| #include "workerd/io/io-gate.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include "workerd/io/io-gate.h" | |
| #include <workerd/io/io-gate.h> |
|
|
||
| void inputGateLocked() override { | ||
| metrics.inputGateLocked(); | ||
| if (IoContext::hasCurrent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should always have an active IoContext shouldn't we?
So
auto& context = IoContext::current();
auto inputGateHoldSpan = context.makeTraceSpan("actor_input_gate_hold"_kjc);Should be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my testing, we don't have IoContext when we invoke the input gate or hold them. I am trying to see if I can use the parent's context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would really limit the effectiveness and reliability of this PR.
I don't understand input gates well enough but I would love to learn more about them to understand how to best approach this.
If we need to instead iterate over all of the actor's current inflight requests and open spans in all of them we could do that instead of relying on the IoContext
| metrics.outputGateWaiterAdded(); | ||
| if (IoContext::hasCurrent()) { | ||
| auto& ioContext = IoContext::current(); | ||
| outputGateWaitSpan = ioContext.makeTraceSpan("actor_output_gate_wait"_kjc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is naively overwriting any existing span, but it's possible for an OutputGate to have multiple waiters. I'll look more closely in a minute, but I'd expect us to want to attach a span to each (traced) waiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd think we could hold these WaitSpan variables in the InputGate::Waiter for InputGates, and for OutputGates we could attach a span on the branch promise returned from OutputGate::wait (similar to the defer for hooks.outputGateWaiterRemoved())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with one request and this works, but or input gates the IoContext is not set at the point where the gates are held/waited, so have to use the parent's IoContext or pass in a parent span. I am working on that change now, will mostly switch to same model for outpute gate too
|
Why is this the right layer to create the spans? It certainly seems awkward to have to keep track of the spans at the hook layer. It seems to me that a better approach might be to take SpanParent parameters to (I think @a-robinson said more or less the same thing in #5563 (comment).) |
Yes, see my reply above for Alex's comment |
No description provided.