Instrument jsrpc#6750
Conversation
![ask-bonk[bot]](https://avatars.gh.lumenfield.work/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds per-call jsRpcCall trace spans on both client and server sides of JS-RPC, with origin propagation through returned stubs, and nests inner operation spans under the factory's outer dispatch span.
- [LOW] Misleading comment:
SpanParentholdskj::Own<SpanObserver>(refcounted), so these are owned references, not borrowed. The "valid for the lifetime ofclient" constraint is inaccurate and could cause confusion for future maintainers.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted on PR #6750. Summary of findings:
|
danlapid
force-pushed
the
dlapid/jsrpcTracing
branch
2 times, most recently
from
58edf06 to
53debf7
Compare
|
The generated output of |
…tinuity
Adds a `jsRpcCall` user trace span on every JS-RPC dispatch, on both sides
of the wire, so that each method invocation is visible as its own span in
tail traces. Spans are tagged with `jsrpc.method`, `jsrpc.target_kind`
(fetcher / entrypoint / stub / transient / promise), and `jsrpc.operation`
(call / getProperty).
Stub origin propagation: stubs returned via RPC now carry the
`jsRpcCall` of the call that produced them. Follow-up calls on those
stubs nest under the originating call rather than under the request
root, restoring trace continuity across pipelined RPC chains. Plumbed
through `RpcDeserializerExternalHandler`, `JsRpcStub`, and a new
`JsRpcClientProvider::ClientForOneCall { client, callSpanParents }`
return shape from `getClientForOneCall`.
OutgoingFactory result redesign: `Fetcher::OutgoingFactory::Result` now
returns `{ client, spanParents }` so `Fetcher::buildClient` can nest its
inner operation span under the factory's outer dispatch span (e.g.
`durable_object_subrequest`). This is what gives `durable_object_subrequest`
correct attribution as a parent of the per-call spans inside a DO stub
invocation. Cascades through actor.{h,c++}, actor-state.c++,
container.c++, sockets.c++ (the latter two return `spanParents = kj::none`
since they don't open an outer dispatch span). New `TraceContextParent`
helper in `trace.h` carries a borrowed (internalSpan, userSpan) pair
without owning SpanBuilders.
Server side: `JsRpcSessionCustomEvent::run` emits an internal-only
`jsRpcSession` span for the legacy buffered tail. No user-visible
`jsRpcSession` is emitted on the server because the jsrpc-typed onset
already represents the session (delivered() to outcome).
Test fixtures updated: tail-worker-test gains `jsRpcCall` open/close
events with the new tags on each callee, jsrpcDoSubrequest reflects the
nested `jsRpcSession -> jsRpcCall -> durable_object_subrequest ->
jsRpcSession -> jsRpcCall` shape, sql-test-tail and actor-kv-test-tail
adjust span counts, and `runInstrumentationTest` filters `jsRpcCall`
alongside `jsRpcSession` by default.
danlapid
force-pushed
the
dlapid/jsrpcTracing
branch
from
53debf7 to
69596a1
Compare
|
|
||
| // Server-side jsRpcCall, attached to the dispatch promise below so it stays | ||
| // open through JS invocation and result serialization. | ||
| auto jsRpcCallSpan = ctx.makeUserTraceSpan("jsRpcCall"_kjc); |
There was a problem hiding this comment.
The callee's per call span should parent to the caller's per call client span, not to the session. This separates per call spans from the invocation onset, but matches convention and the user's mental model. Achievable by carrying span context on CallParams.
There was a problem hiding this comment.
Yeah sounds like a good idea but let me think about it more
It'll be the first time we move from having all spans be hierarchical children of the onset.
I don't know the ramifications..
| TraceContextParent parents = kj::mv(callSpanParents).orDefault([&] { | ||
| return TraceContextParent(ioContext.getCurrentTraceSpan(), ioContext.getCurrentUserTraceSpan()); | ||
| }); | ||
| TraceContext span = parents.newChild("jsRpcCall"_kjc); |
There was a problem hiding this comment.
Use method name as the span name not fixed jsRpcCall. Both sides.
There was a problem hiding this comment.
We don't do this elsewhere, we use constant span names and let the OTEL ingestor visualize however it wants.
There was a problem hiding this comment.
The span name should be rpc method per the convention. Better to break a incorrect pattern.
| }); | ||
| TraceContext span = parents.newChild("jsRpcCall"_kjc); | ||
|
|
||
| span.setTag("jsrpc.target_kind"_kjc, parent.getRpcTargetKind()); |
There was a problem hiding this comment.
- We're not yet emitting span kind, refactoring the repo to use it is quite out of scope.
- target kind is more than just client/server, we will want this regardless of span kind.
There was a problem hiding this comment.
I don't think customer's actually care about target kind. When would they think in those terms? They only care about side and the convention specifies it as span kind.
It's a small change comparable to trace flags.
3 participants
Adds a
jsRpcCalluser trace span on every JS-RPC dispatch, on both sides of the wire, so that each method invocation is visible as its own span in tail traces. Spans are tagged withjsrpc.method,jsrpc.target_kind(fetcher / entrypoint / stub / transient / promise), andjsrpc.operation(call / getProperty).Stub origin propagation: stubs returned via RPC now carry the
jsRpcCallof the call that produced them. Follow-up calls on those stubs nest under the originating call rather than under the request root, restoring trace continuity across pipelined RPC chains. Plumbed throughRpcDeserializerExternalHandler,JsRpcStub, and a newJsRpcClientProvider::ClientForOneCall { client, callSpanParents }return shape fromgetClientForOneCall.OutgoingFactory result redesign:
Fetcher::OutgoingFactory::Resultnow returns{ client, spanParents }soFetcher::buildClientcan nest its inner operation span under the factory's outer dispatch span (e.g.durable_object_subrequest). This is what givesdurable_object_subrequestcorrect attribution as a parent of the per-call spans inside a DO stub invocation. Cascades through actor.{h,c++}, actor-state.c++, container.c++, sockets.c++ (the latter two returnspanParents = kj::nonesince they don't open an outer dispatch span). NewTraceContextParenthelper intrace.hcarries a borrowed (internalSpan, userSpan) pair without owning SpanBuilders.Server side:
JsRpcSessionCustomEvent::runemits an internal-onlyjsRpcSessionspan for the legacy buffered tail. No user-visiblejsRpcSessionis emitted on the server because the jsrpc-typed onset already represents the session (delivered() to outcome).Test fixtures updated: tail-worker-test gains
jsRpcCallopen/close events with the new tags on each callee, jsrpcDoSubrequest reflects the nestedjsRpcSession -> jsRpcCall -> durable_object_subrequest -> jsRpcSession -> jsRpcCallshape, sql-test-tail and actor-kv-test-tail adjust span counts, andrunInstrumentationTestfiltersjsRpcCallalongsidejsRpcSessionby default.