Skip to content

Commit 3fba9db

Browse files
crysmagsBridgeAR
authored andcommitted
refactor(graphql): migrate shimmer to orchestrion instrumentation (#7757)
feat(graphql): migrate instrumentation to orchestrion Migrates GraphQL instrumentation from shimmer wrappers to orchestrion AST rewriting for graphql execute / parse / validate entry points, including CJS and ESM paths for graphql >=0.10 and @graphql-tools/executor. Moves resolver instrumentation into the GraphQL execute plugin. The execute plugin now owns per-execute root context, resolver wrapping, resolve-span lifecycle, source tracking, and resolver hook invocation. The old separate resolve plugin is removed. Preserves and tests the existing cross-feature contracts: - IAST still receives one apm:graphql:resolve:start publish per resolver call, using the actual GraphQL args object. - AppSec still receives resolver payloads through datadog:graphql:resolver:start and can abort synchronously through the shared abort controller. - depth only limits resolve-span creation; IAST/AppSec resolver publishes still happen for depth-gated fields. - depth-gated resolvers now honor abort signals before falling through the no-span fast path. - caller-owned execute args and contextValue are preserved without mutation. - default field resolver behavior matches graphql for primitive parent values. - graphql-yoga / @graphql-tools/executor execution is instrumented. Adds public TypeScript declarations for the GraphQL resolve hook and FieldContext payload. Keeps the implementation orchestrion-only, with no shimmer fallback, and updates the GraphQL long benchmark calibration for the migrated hot path. Regression coverage was added for: - resolver abort behavior past the configured depth - depth: 0 AppSec resolver-channel publishing - primitive-source defaultFieldResolver parity - caller-supplied and frozen execute args - primitive contextValue forwarding - Yoga normalized executor instrumentation - IAST/AppSec per-resolver channel cardinality Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
1 parent 18aa29c commit 3fba9db

19 files changed

Lines changed: 1343 additions & 727 deletions

File tree

.agents/skills/apm-integrations/references/orchestrion.md

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,95 @@ For integrations wrapping multiple methods, create a separate plugin class per m
208208
- `ctx.error` — thrown error (on error)
209209
- `ctx.currentStore` — set by `startSpan` in `bindStart`
210210
211+
## Propagating Synchronous Errors From `bindStart`
212+
213+
Subscribers on the prefix `:start` channel and `bindStore` transforms **cannot
214+
propagate a synchronous throw** to the caller of an orchestrion-wrapped
215+
function. Both are wrapped in `try { ... } catch (err) { process.nextTick(() =>
216+
triggerUncaughtException(err)); ... }` by Node's `diagnostics_channel` (see
217+
`lib/diagnostics_channel.js`'s `wrapStoreRun` and `publish`). The error
218+
surfaces async as an uncaught exception, **after** the wrapped fn has already
219+
run and the call has returned normally.
220+
221+
The wrapper's own catch block, however, **does** rethrow:
222+
223+
```js
224+
return ch.start.runStores(__apm$ctx, () => {
225+
try {
226+
const result = __apm$traced();
227+
__apm$ctx.result = result;
228+
return result;
229+
} catch (err) {
230+
__apm$ctx.error = err;
231+
ch.error.publish(__apm$ctx);
232+
throw err; // <- propagates the wrapped fn's error to the caller
233+
} finally {
234+
ch.end.publish(__apm$ctx);
235+
}
236+
});
237+
```
238+
239+
When a contract requires `assert.throws(() => wrapped(...))`-style synchronous
240+
propagation from a `:start` observer (the canonical case is AppSec WAF's
241+
`abortController.abort()` model), use the **Proxy-on-arguments pattern**:
242+
243+
```js
244+
bindStart (ctx) {
245+
// ... normal setup, span creation ...
246+
const abortController = new AbortController()
247+
if (startCh.hasSubscribers) {
248+
startCh.publish({ abortController, args }) // subscribers run sync
249+
if (abortController.signal.aborted) {
250+
// ctx.arguments is the SAME array reference the wrapper spreads into
251+
// the wrapped fn (__apm$wrapped.apply(this, ctx.arguments)). Replace
252+
// arguments[0] with a Proxy whose getters throw AbortError. The
253+
// wrapped fn's first property access (typically a destructure of args)
254+
// triggers the trap → the wrapper's catch+rethrow propagates AbortError
255+
// to the caller. Span lifecycle still completes via end.publish.
256+
ctx.arguments[0] = new Proxy({}, {
257+
get () { throw new AbortError('Aborted') },
258+
has () { throw new AbortError('Aborted') },
259+
})
260+
ctx.ddAborted = true
261+
return ctx.currentStore
262+
}
263+
}
264+
// ... rest of bindStart ...
265+
}
266+
267+
error (ctx) {
268+
if (ctx.ddAborted) return // abort != error tag
269+
// ... regular error handling ...
270+
}
271+
```
272+
273+
Reference implementation: `packages/datadog-plugin-graphql/src/execute.js`
274+
(`apm:graphql:execute:start` contract).
275+
276+
Why this works:
277+
- `ctx.arguments` and the rewriter's `__apm$arguments` are the same array
278+
reference (confirmed by capturing the rewriter output: `const __apm$traced
279+
= () => __apm$wrapped.apply(this, __apm$arguments)`).
280+
- The wrapped fn's body almost always touches `arguments[0]` on the first
281+
statement (destructure, property read, validation). Any read triggers the
282+
Proxy trap.
283+
- The orchestrion wrapper's `catch { ...; throw err }` propagates the thrown
284+
error synchronously to the caller — confirmed in the rewriter template
285+
(vendor's `code-transformer/index.js`, `wrapSync` function).
286+
- `:end` still fires in `finally`, so the plugin's `end(ctx)` runs as usual
287+
and the span lifecycle completes cleanly. Combined with an `error(ctx)`
288+
that no-ops when `ctx.ddAborted`, the span finishes with `error === 0`
289+
(matches the abort contract).
290+
291+
When NOT to use this:
292+
- If you control the wrapped fn (e.g., it's a method you can wrap with
293+
shimmer on top of orchestrion), do that — clearer and avoids the
294+
Proxy indirection.
295+
- If the wrapped fn might catch its own errors before they escape (some
296+
user-resolver patterns do this), the AbortError won't propagate. Verify
297+
the wrapped fn does NOT have a top-level `try/catch` around the
298+
property access that triggers the trap.
299+
211300
## Common Issues
212301
213302
### Wrong filePath

benchmark/sirun/plugin-graphql-long/meta.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
"env": { "WITH_TRACER": "1", "WITH_DEPTH": "0", "OPERATIONS": "1150" }
1212
},
1313
"with-depth-on-max": {
14-
"env": { "WITH_TRACER": "1", "WITH_DEPTH": "4", "OPERATIONS": "530" }
14+
"env": { "WITH_TRACER": "1", "WITH_DEPTH": "4", "OPERATIONS": "800" }
1515
},
1616
"with-depth-and-collapse-off": {
1717
"env": { "WITH_TRACER": "1", "WITH_DEPTH_AND_COLLAPSE": "4,0", "OPERATIONS": "210" }

docs/test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,8 @@ const graphqlOptions: plugins.graphql = {
230230
hooks: {
231231
execute: (span?: Span, args?, res?) => { },
232232
validate: (span?: Span, document?, errors?) => { },
233-
parse: (span?: Span, source?, document?) => { }
233+
parse: (span?: Span, source?, document?) => { },
234+
resolve: (span?: Span, field?) => { }
234235
}
235236
};
236237

index.d.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2491,6 +2491,18 @@ declare namespace tracer {
24912491
typeResolver?: any,
24922492
}
24932493

2494+
/** Context object passed to the `hooks.resolve` callback for each instrumented field. */
2495+
interface FieldContext {
2496+
/** The field name being resolved */
2497+
fieldName: string;
2498+
/** The dot-separated field path (e.g. `'user.address.city'`) */
2499+
path: string;
2500+
/** The error from the resolver, or `null` if it succeeded */
2501+
error: Error | null;
2502+
/** The value returned by the resolver (sync resolvers only; `undefined` for async) */
2503+
result: unknown;
2504+
}
2505+
24942506
/**
24952507
* This plugin automatically instruments the
24962508
* [graphql](https://github.com/graphql/graphql-js) module.
@@ -2569,6 +2581,7 @@ declare namespace tracer {
25692581
execute?: (span?: Span, args?: ExecutionArgs, res?: any) => void;
25702582
validate?: (span?: Span, document?: any, errors?: any) => void;
25712583
parse?: (span?: Span, source?: any, document?: any) => void;
2584+
resolve?: (span?: Span, field?: FieldContext) => void;
25722585
}
25732586
}
25742587

index.d.v5.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2625,6 +2625,18 @@ declare namespace tracer {
26252625
typeResolver?: any,
26262626
}
26272627

2628+
/** Context object passed to the `hooks.resolve` callback for each instrumented field. */
2629+
interface FieldContext {
2630+
/** The field name being resolved */
2631+
fieldName: string;
2632+
/** The dot-separated field path (e.g. `'user.address.city'`) */
2633+
path: string;
2634+
/** The error from the resolver, or `null` if it succeeded */
2635+
error: Error | null;
2636+
/** The value returned by the resolver (sync resolvers only; `undefined` for async) */
2637+
result: unknown;
2638+
}
2639+
26282640
/**
26292641
* This plugin automatically instruments the
26302642
* [graphql](https://github.com/graphql/graphql-js) module.
@@ -2703,6 +2715,7 @@ declare namespace tracer {
27032715
execute?: (span?: Span, args?: ExecutionArgs, res?: any) => void;
27042716
validate?: (span?: Span, document?: any, errors?: any) => void;
27052717
parse?: (span?: Span, source?: any, document?: any) => void;
2718+
resolve?: (span?: Span, field?: FieldContext) => void;
27062719
}
27072720
}
27082721

0 commit comments

Comments
 (0)