feat: binding span enrichment via jsRpc (v1)#6695
feat: binding span enrichment via jsRpc (v1)#6695Ankcorn wants to merge 19 commits intotankcorn/jsrpc-span-parentingfrom
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds ctx.tracing.setJsrpcSessionAttributes() allowing a trusted callee to enrich the caller's jsRpcSession user span with a rename and attributes, gated by allowBindingSpanEnrichment on ServiceDesignator.
Issues found (ranked by severity):
- [MED] Stale TODO in test config — says the capnp field doesn't exist yet, but it was already added in this PR.
- [MED] Orphaned
onUpdateNamedoc comments intrace.h— the method was removed but two comments still reference it. - [LOW] Stale comment in
http.c++referencing~TraceContext reads pendingEnrichment—PendingEnrichmentonTraceContextwas removed in a later commit in this PR.
This review was generated by an AI assistant and may contain inaccuracies.
| # Service binding with spanEnrichmentPolicy. | ||
| # TODO(impl): allowBindingSpanEnrichment field does not exist yet in workerd.capnp; | ||
| # add it as part of the implementation of this feature. |
There was a problem hiding this comment.
This TODO is stale — allowBindingSpanEnrichment already exists in workerd.capnp (added in this PR, at ServiceDesignator @4). The comment should be cleaned up.
| # Service binding with spanEnrichmentPolicy. | |
| # TODO(impl): allowBindingSpanEnrichment field does not exist yet in workerd.capnp; | |
| # add it as part of the implementation of this feature. | |
| # Service binding with allowBindingSpanEnrichment. |
| // Called when the operation name is changed after the span was opened (via | ||
| // SpanBuilder::setOperationName()). Observers that eagerly stream the open event should handle | ||
| // this; others may simply update their buffered state. Default implementation is a no-op. |
There was a problem hiding this comment.
Orphaned doc comment — the onUpdateName() virtual method it describes was removed in this PR, but these three comment lines were left behind. The class-level doc comment at line 1156 also still references onUpdateName() (though that line is outside the diff hunk). At minimum these orphaned lines should be deleted.
| // Called when the operation name is changed after the span was opened (via | |
| // SpanBuilder::setOperationName()). Observers that eagerly stream the open event should handle | |
| // this; others may simply update their buffered state. Default implementation is a no-op. |
|
Review posted successfully on PR #6695. I found 3 actionable issues — all are stale/orphaned comments from the multi-commit development history, with suggested fixes provided inline. The core design (trust model, capnp schema additions, raw pointer lifetime, enrichment gating) looks sound. |
|
The generated output of Full Type Diffdiff -r types/generated-snapshot/latest/index.d.ts bazel-bin/types/definitions/latest/index.d.ts
4062a4063
> enrichBindingSpan(options: TracingEnrichmentOptions): void;
4063a4065,4068
> }
> interface TracingEnrichmentOptions {
> name?: string;
> attributes?: Record<string, any>;
diff -r types/generated-snapshot/latest/index.ts bazel-bin/types/definitions/latest/index.ts
4068a4069
> enrichBindingSpan(options: TracingEnrichmentOptions): void;
4069a4071,4074
> }
> export interface TracingEnrichmentOptions {
> name?: string;
> attributes?: Record<string, any>; |
Merging this PR will not alter performance
Comparing Footnotes
|
5e2c54f to
bca3c14
Compare
770829d to
ea0f0d7
Compare
0fe59fb to
fdc5681
Compare
ea0f0d7 to
49ab361
Compare
fdc5681 to
04aa1ca
Compare
49ab361 to
03931c0
Compare
04aa1ca to
685b7fa
Compare
03931c0 to
5ec3e52
Compare
Adds ctx.tracing.setBindingSpan() — a callee-side API that lets a worker
write attributes onto the caller's jsRpcSession user span. The runtime
ships the buffered enrichment back over the existing RPC return path
(BindingSpanEnrichment field on JsRpcTarget.CallResults) and the caller
applies the attributes as tags on the span. The special key 'span.name'
is forwarded as a 'span.name' attribute event; the tail worker is
responsible for treating it as a rename.
Built on tankcorn/jsrpc-span-parenting:
- The jsRpcSession SpanBuilder is owned by JsRpcSessionCustomEvent.
- JsRpcSessionCustomEvent is constructed in callImpl(), which has
direct access to the span without any pointer-out-of-the-function.
- Enrichment is applied in callImpl()'s response lambda by capturing
the SpanBuilder pointer; lifetime is the session lifetime, enforced
by the addTask promise that owns the event.
No new public Fetcher, JsRpcClientProvider, or callImpl signature
changes; only the wire format, the callee-side API, and the buffered
storage on IoContext::IncomingRequest are new.
685b7fa to
b367ed8
Compare
5ec3e52 to
bf2e88c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tankcorn/jsrpc-span-parenting #6695 +/- ##
=================================================================
+ Coverage 66.54% 66.59% +0.04%
=================================================================
Files 402 402
Lines 115902 116036 +134
Branches 19412 19439 +27
=================================================================
+ Hits 77131 77270 +139
+ Misses 27188 27170 -18
- Partials 11583 11596 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nrichment # Conflicts: # src/workerd/api/worker-rpc.c++
- Rename ctx.tracing.setBindingSpan -> ctx.tracing.enrichBindingSpan. The method enriches an existing span (the caller's binding-call span) with attributes; "set" misleadingly suggests creating or replacing a span object. Naming aligns with the surrounding vocabulary used everywhere internally (BindingSpanEnrichment capnp struct, pendingBindingSpanEnrichment, etc.). - B1: guard int64 coercion. JS Numbers can be NaN, +/-Infinity, or arbitrary doubles outside int64 range; static_cast<int64_t> on those is undefined behaviour. Now we only coerce to int64 when the value is finite, in [INT64_MIN, INT64_MAX), and exactly representable as an integer. Otherwise the value passes through as a double. - S1: cap input. Per-key length 256 B, per-value length 128 KB, max 256 attributes per call. Entries that exceed any cap are silently dropped so a misbehaving callee can't brick its caller. AIG callers may attach AI context payloads, hence the relatively large per-value cap; we can revisit if real workloads need more.
…tive tests
- B2: 'span.name' is reserved; always consume the key. Previously a
non-string value (e.g. setBindingSpan({'span.name': 42})) would fall
through to the regular-attribute branch and tag the span with a key
literally named 'span.name', which is surprising. Now non-string
values for the reserved key are silently ignored as documented.
- D8: document that enrichBindingSpan only takes effect on direct
service-binding calls (env.svc.method()). Held-stub calls
(await s.method()) buffer enrichment correctly on the callee side
but the caller has no span to enrich on those paths so the data is
dropped on receive. Also explicit no-op outside an RPC method.
- N1, N2: drop the stale '# TDD tests / Test 1 / Test 2 /
spanEnrichmentPolicy' BUILD comment; the SpanEnrichmentPolicy idea
was dropped in favour of a public API per Dan, and only one test
exists.
- N8 negative tests: a runEdgeCases callee method exercises the B2
guard (non-string span.name must not rename the span) and the B1
guard (Infinity / NaN must round-trip as numbers without crashing
or hitting the int64 cast).
Replaces the magic-string design (a flat attributes dict where
"span.name" was specially extracted as a rename) with a structured
options object: { name, attributes }. `name` is required because every
enrichment renames the span -- a span called "jsRpcSession" is
meaningless to a user reading their tail stream. `attributes` is
optional.
Before:
ctx.tracing.enrichBindingSpan({
'span.name': 'ai_gateway.run',
'gen_ai.request.model': model,
});
After:
ctx.tracing.enrichBindingSpan({
name: 'ai_gateway.run',
attributes: { 'gen_ai.request.model': model },
});
Benefits:
- No collision with real OTel attribute keys named "span.name".
- No lossy round-trip: capnp wire already had separate `name` and
`attributes` fields; the JS API now matches.
- Required `name` forces the callee to give a meaningful operation
name ("ai_gateway.run", "d1.query", etc.) rather than tagging
attributes onto a generic "jsRpcSession".
- Drops the always-consume / non-string-silent-drop guard in C++
(no more reserved key).
Also drops the corresponding negative test case in the tail worker
(non-string "span.name" can no longer happen because `name` is typed
as kj::String at the JSG boundary).
Previously each call to ctx.tracing.enrichBindingSpan() replaced any previously buffered enrichment for the in-flight method (last-call-wins at the bag level). Two calls in the same method would silently lose the first one's attributes. Now successive calls merge: - name: latest call wins (still single-valued). - attributes: upsert by key. Same-key entries replace the prior value; new keys append (subject to the 256-attribute count cap). Implementation reuses takePendingBindingSpanEnrichment to atomically pull the existing buffer, merges in the new data, and writes the combined bag back. AIG and similar use cases naturally accumulate attributes across the lifetime of an RPC method (e.g. set the model up front, append token counts when the upstream response arrives), so additive semantics are what users expect. Test: callMerge / runMerge exercises overwrite-by-key, append-new-key, and survives-from-first-call paths.
With merge semantics, requiring `name` on every call forces callees
to repeat themselves on attribute-only updates:
enrichBindingSpan({ name: 'ai_gateway.run', attributes: {...} });
// ... do upstream call ...
enrichBindingSpan({ name: 'ai_gateway.run', attributes: {...} }); // <-- redundant
Both fields are now optional. Typical pattern: set `name` and initial
attributes up front, then call again with only `attributes` to append
more. If no call ever sets `name`, the span keeps its default
"jsRpcSession" name (silent no-op rename) -- documented as suboptimal
in the JSDoc.
Test extended to exercise the attributes-only pattern: third call
appends a new attribute and asserts the `name` from the previous call
is preserved.
…nrichment # Conflicts: # src/workerd/api/worker-rpc.c++
…nrichment # Conflicts: # src/workerd/api/worker-rpc.c++
…nrichment # Conflicts: # src/workerd/api/worker-rpc.c++
…nrichment # Conflicts: # src/workerd/api/worker-rpc.h
…nrichment # Conflicts: # src/workerd/api/worker-rpc.c++
What
New JS API:
ctx.tracing.enrichBindingSpan({ name, attributes }). Lets a callee rename and tag the caller's binding-call span. Works for any jsRpc binding — AIG isthe first user but the mechanism is generic (D1, Vectorize, your service binding, anything reachable via
env.x.method()).Both fields are optional, but you should set
nameat least once — a span calledjsRpcSessionis meaningless to a user reading their tail stream. Multiple callsin one method merge: latest
namewins, attributes upsert by key.How
Buffered on the callee's
IncomingRequest, drained into a new optionalbindingSpanEnrichmentfield onJsRpcTarget.CallResults, applied in the caller's responselambda to the in-scope
jsRpcSessionspan. STW sees attribute events and a rename.Caveats
env.svc.method()). Held-stub calls buffer correctly but the caller has no span to enrich on that path. Documented in JSDoc.NaN/Infinityskip the int64 cast.Promise.allis isolated.Open questions
JsRpcSessionCustomEvent::jsRpcSessionSpan(currently public socallImplcan take its address)?