Skip to content

refactor(graphql): migrate shimmer to orchestrion instrumentation#7757

Merged
crysmags merged 27 commits into
masterfrom
crysmags/graphql-migration
Jun 27, 2026
Merged

refactor(graphql): migrate shimmer to orchestrion instrumentation#7757
crysmags merged 27 commits into
masterfrom
crysmags/graphql-migration

Conversation

@crysmags

@crysmags crysmags commented Mar 12, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Migrates the graphql plugin's instrumentation strategy from shimmer.wrap to orchestrion AST rewriting. The plugin no longer dynamically wraps individual field resolvers at execute time; instead orchestrion rewrites graphql's execute, parse, and validate entry points at module load to publish on diagnostic channels. The plugin subscribes to those channels and owns the per-execute / per-resolver work in packages/datadog-plugin-graphql/src/execute.js.

Also wires up two public API additions (FieldContext type + hooks.resolve user callback) and adds one benchmark variant for measuring shared dd-trace startup cost separately from graphql plugin overhead.


Commits

# commit what it does
1 feat(graphql): migrate from shimmer to orchestrion instrumentation The full migration: orchestrion config, plugin restructure, IAST/AppSec correctness fixes, perf optimizations, depth=0 fast path
2 feat(graphql): expose FieldContext and hooks.resolve public types Public TypeScript declarations for the new resolve hook
3 bench(graphql-long): add tracer-loaded graphql-disabled baseline variant + microbench harness Adds a with-tracer-graphql-disabled variant for shared-cost subtraction, plus a microbench harness for local plugin work

Migration overview

Before (master)

  • packages/datadog-instrumentations/src/graphql.js uses shimmer.wrap against graphql.execute, then walks the schema's type map at execution time to wrap every field resolver
  • Per-field wrap fires for every resolver call, regardless of whether a span will be created
  • Separate GraphQLResolvePlugin sub-class handles resolver spans via channel subscriptions

After (this PR)

  • packages/datadog-instrumentations/src/helpers/rewriter/instrumentations/graphql.js (new) — orchestrion config declaring which functions to rewrite (execute, parse, validate) for both graphql (CJS + ESM, versions ≥0.10) and @graphql-tools/executor (for graphql-yoga)
  • packages/datadog-instrumentations/src/graphql.js — stripped to just addHook registrations + module-export captures for the IAST taint-tracking signature tools (printer, visitor, utilities)
  • packages/datadog-plugin-graphql/src/execute.js — owns the migrated execute and resolver work in one plugin. Resolver wrapping happens at the schema-walk stage of the first execute; resolve spans are created inline at first-encounter and finished from the resolver's then-callback so encoder state stays hot.
  • packages/datadog-plugin-graphql/src/resolve.js — deleted. The separate resolve plugin was needed by the old shimmer architecture (per-field channel publishes routed through a different plugin); with the orchestrion design the work folds into the execute plugin and the tracer.use('graphql', { depth: 0 }) short-circuit is handled by a module-level _depthDisabled flag set in configure().

Correctness fixes folded into the migration commit

AppSec resolver blocking — channel publish payload aligned with the AppSec subscriber's destructure. The old shimmer path sent { ctx, resolverInfo } but the AppSec handler destructured { context, ... }, so blocking never actually aborted a resolve. New payload is { abortController, resolverInfo } and the handler calls abortController?.abort() directly.

IAST taint tracking for hardcoded resolver args — initial orchestrion port published a reconstructed resolverArgs object built from AST nodes. taintObject() mutated that object, but graphql's getArgumentValues() builds its own separate args object inside executeField, so the taint never reached the resolver body. Fix: when apm:graphql:resolve:start has subscribers, temporarily replace fieldDef.resolve with a wrapper that fires the IAST channel with the actual args object graphql constructs internally, restoring the wrapper synchronously as its first action (no async gap between bindStart and the resolver call).

AppSec request lookuppackages/dd-trace/src/appsec/graphql.js now reads req via getActiveRequest() instead of storage('legacy').getStore()?.req. The legacy storage call only worked under the old shimmer setup; under orchestrion it would have hit a ReferenceError because the helper was never imported.

Performance work folded in

  • Lazy-build info and resolver args only when the plugin needs them
  • Eliminate per-field channel publishing — the per-execute rootCtx holds field records, no per-resolver publish in the APM-only path
  • Collapse redundant path walks: pathToArray runs once per resolver
  • Consolidate per-field stash + finish handler into one shared object
  • Re-entrant execute() short-circuit (covers yoga's normalizedExecutor pattern)
  • depth=0 fast path: module-level _depthDisabled flag set in configure() so resolveAsync bails on a single property read (matching master's startResolveCh.hasSubscribers cost shape)
  • Picks up master's signature memoization (tools/signature.js WeakMap cache) via this PR's rebase

Public API additions (commit 2)

// New in index.d.ts:
interface FieldContext {
  fieldName: string;   // e.g. 'hello'
  path: string;        // dot-separated path, e.g. 'user.address.city'
  error: Error | null; // resolver error, or null on success
  result: unknown;     // sync resolver return value; undefined for async
}

interface plugins.graphql.hooks {
  resolve?: (span?: Span, field?: FieldContext) => void;
  // (execute, parse, validate hooks unchanged)
}

Usage:

tracer.use('graphql', {
  hooks: {
    resolve (span, field) {
      span.setTag('app.field', field.fieldName)
    }
  }
})

Hook fires once per resolved field (post-resolve, before span.finish()), giving users a place to add custom tags or inspect resolver results without subscribing directly to the diagnostic channel.


Bench changes (commit 3)

Adds with-tracer-graphql-disabled variant to plugin-graphql-long:

// New variant:
WITH_TRACER=1 GRAPHQL_DISABLED=1
// → tracer.init() + tracer.use('graphql', false)

Behaviour equivalent on master (uses shimmer; explicit-false unsubscribes) and on PR 7757 (orchestrion publish goes to no subscribers). Comparing other tracer-on variants against this one isolates plugin-specific overhead from shared dd-trace startup cost.

Also adds microbench.js, a standalone harness that microbenchmarks the plugin's hot-path functions for fast local iteration (not picked up by sirun, which only runs meta.json variants).


Performance results

plugin-graphql-long benchmark (100 queries/process, 5 sirun iterations, Node 18/20/22/24). All numbers are per-process user-CPU on PR vs master:

variant direction notes
with-depth-on-max (typical production config) 🟩 −13 to −36% large per-resolver wins
with-depth-and-collapse-on 🟩 −13 to −33% large per-resolver wins
with-depth-and-collapse-off (1700 spans/qry) 🟩 ~tied to small win per-resolver savings balance encoding cost
with-tracer-graphql-disabled (new) 🟨 tied (~±50ms) shared dd-trace startup is fine
with-depth-off (resolver tracing explicitly off) 🟥 +5-9% on Node 22 structural orchestrion cost; see below

Why with-depth-off-22 regresses

Orchestrion's publish format carries raw function arguments. Master's shimmer wrapper pre-computed { operation, args, docSource } before publishing, so master's bindStart got them for free. PR 7757's bindStart re-extracts them per call (readArgs, getOperation, documentSources.get). For depth=0 (no resolve spans created), this per-execute payload extraction is the only thing happening, so it dominates the variant.

For any configuration with resolver work (the other tracer-on variants), per-resolver wins overtake this cost many times over.

Local measurement confirmed the subtraction:

  • master depth=0 ≈ master tracer-graphql-disabled (≈0 plugin work)
  • PR 7757 depth=0 = PR tracer-graphql-disabled + 130ms per process

The +130ms / process on depth=0 vs master's effectively-zero is the cost. Master's resolve plugin self-disables via super.configure(false) when depth=0 and its execute plugin gets a pre-computed payload from shimmer's wrap — together producing near-zero per-execute work. Neither is available on the orchestrion architecture without restructuring orchestrion's publish payload itself, which is out of scope.

What this regression doesn't affect

  • tracer.use('graphql', { depth: 0 }) still emits graphql.execute spans and skips graphql.resolve spans, identical to master's behaviour
  • Real production graphql traffic runs at depth>0 and gets the wins from other variants
  • dd-trace's startup cost is roughly equivalent between branches (the with-tracer-graphql-disabled variant ties between master and PR)

Related but separate

startup-with-tracer-everything (a separate bench, not in plugin-graphql-long) shows +8-13% on tracer-init cost when every integration's hooks register. PR 7757's orchestrion AST rewriting of graphql at module load is the most likely contributor. Worth flagging here even though it's outside the graphql query-time question.


Test results

  • 223 graphql plugin tests passing, 0 failing (graphql v15, v16; CJS + ESM)
  • 9/9 AppSec graphql unit tests passing
  • 8/8 IAST graphql source tests passing (hardcoded + variable args, all apollo-server-express versions)

🤖 Generated with Claude Code

@codecov

codecov Bot commented Mar 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.69452% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.67%. Comparing base (1ed6244) to head (dcea155).

Files with missing lines Patch % Lines
packages/datadog-plugin-graphql/src/execute.js 97.98% 6 Missing ⚠️
packages/datadog-plugin-graphql/src/parse.js 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7757      +/-   ##
==========================================
- Coverage   93.69%   93.67%   -0.03%     
==========================================
  Files         889      889              
  Lines       50846    50856      +10     
  Branches    11801    11830      +29     
==========================================
- Hits        47640    47637       -3     
- Misses       3206     3219      +13     
Flag Coverage Δ
aiguard 34.88% <100.00%> (-0.07%) ⬇️
aiguard-integration 41.79% <100.00%> (+<0.01%) ⬆️
apm-bucket-0 34.85% <100.00%> (-0.08%) ⬇️
apm-bucket-1 40.34% <100.00%> (-0.07%) ⬇️
apm-bucket-2 37.35% <100.00%> (-0.07%) ⬇️
apm-capabilities-tracing 48.29% <6.84%> (-0.28%) ⬇️
apm-integrations-aerospike 33.13% <100.00%> (-0.07%) ⬇️
apm-integrations-confluentinc-kafka-javascript 40.04% <100.00%> (-0.07%) ⬇️
apm-integrations-couchbase 33.41% <100.00%> (-0.18%) ⬇️
apm-integrations-http 42.04% <40.00%> (-0.07%) ⬇️
apm-integrations-kafkajs 40.27% <100.00%> (-0.07%) ⬇️
apm-integrations-next 29.47% <100.00%> (-0.11%) ⬇️
apm-integrations-prisma 35.04% <100.00%> (-0.07%) ⬇️
apm-integrations-tedious 33.90% <100.00%> (-0.07%) ⬇️
appsec 57.37% <100.00%> (+0.02%) ⬆️
appsec-express_fastify_graphql 53.77% <80.97%> (-0.01%) ⬇️
appsec-integration 36.25% <73.77%> (+0.08%) ⬆️
appsec-kafka_ldapjs_lodash 43.60% <100.00%> (-0.06%) ⬇️
appsec-mongodb-core_mongoose_mysql 48.82% <60.00%> (-0.05%) ⬇️
appsec-next 28.02% <100.00%> (-0.05%) ⬇️
appsec-node-serialize_passport_postgres 48.01% <60.00%> (-0.06%) ⬇️
appsec-sourcing_stripe_template 45.56% <60.00%> (-0.06%) ⬇️
debugger 44.37% <100.00%> (+0.23%) ⬆️
instrumentations-bucket-0 28.16% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-1 37.41% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-10 40.44% <40.00%> (-0.07%) ⬇️
instrumentations-bucket-11 27.96% <100.00%> (-0.08%) ⬇️
instrumentations-bucket-12 28.67% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-13 27.79% <100.00%> (-0.08%) ⬇️
instrumentations-bucket-2 30.24% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-3 35.91% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-4 28.07% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-5 36.28% <100.00%> (-0.06%) ⬇️
instrumentations-bucket-6 38.26% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-7 35.92% <100.00%> (-0.15%) ⬇️
instrumentations-bucket-8 36.96% <100.00%> (-0.07%) ⬇️
instrumentations-bucket-9 39.53% <40.00%> (-0.07%) ⬇️
instrumentations-instrumentation-couchbase 46.54% <100.00%> (+0.03%) ⬆️
instrumentations-integration-esbuild 24.87% <50.00%> (+0.11%) ⬆️
llmobs-ai_anthropic_bedrock 39.54% <100.00%> (-0.06%) ⬇️
llmobs-google-genai_langchain_vertex-ai 36.98% <100.00%> (+0.06%) ⬆️
llmobs-openai 39.59% <100.00%> (-0.07%) ⬇️
llmobs-sdk 43.60% <100.00%> (-0.07%) ⬇️
master-coverage 93.67% <97.69%> (?)
openfeature 37.76% <100.00%> (+<0.01%) ⬆️
openfeature-unit 50.39% <ø> (ø)
platform-core_esbuild_instrumentations-misc 23.36% <50.00%> (+0.10%) ⬆️
platform-integration 47.43% <100.00%> (-0.01%) ⬇️
platform-shimmer_unit-guardrails_webpack 18.89% <50.00%> (+0.09%) ⬆️
plugins-bucket-0 36.34% <100.00%> (-0.06%) ⬇️
plugins-bucket-1 39.61% <100.00%> (+<0.01%) ⬆️
plugins-bucket-11 38.47% <100.00%> (-0.07%) ⬇️
plugins-bucket-17 39.16% <100.00%> (-0.07%) ⬇️
plugins-bucket-18 42.01% <13.99%> (-0.14%) ⬇️
plugins-bucket-19 39.57% <95.91%> (-0.06%) ⬇️
plugins-bucket-20 43.26% <100.00%> (-0.07%) ⬇️
plugins-bucket-4 37.72% <100.00%> (-0.07%) ⬇️
plugins-bullmq_cassandra_cookie 39.77% <100.00%> (-0.07%) ⬇️
plugins-cookie-parser_crypto_dd-trace-api 33.23% <100.00%> (-0.08%) ⬇️
plugins-fetch_fs_generic-pool 36.06% <100.00%> (-0.07%) ⬇️
plugins-google-cloud-pubsub_grpc_handlebars 43.12% <100.00%> (-0.06%) ⬇️
plugins-hapi_hono_ioredis 37.79% <100.00%> (-0.07%) ⬇️
plugins-jest_knex_langgraph 32.61% <100.00%> (-0.07%) ⬇️
plugins-ldapjs_light-my-request_limitd-client 27.85% <100.00%> (-0.08%) ⬇️
plugins-lodash_mariadb_memcached 35.18% <100.00%> (-0.07%) ⬇️
plugins-mongodb_mongodb-core_mongoose 36.32% <100.00%> (-0.07%) ⬇️
plugins-multer_mysql_mysql2 35.15% <100.00%> (-0.07%) ⬇️
plugins-nats_node-serialize_opensearch 37.16% <100.00%> (-0.07%) ⬇️
plugins-passport-http_pino_postgres 35.54% <100.00%> (-0.07%) ⬇️
plugins-process_pug_redis 34.27% <100.00%> (-0.07%) ⬇️
plugins-undici_url_valkey 35.89% <100.00%> (-0.06%) ⬇️
plugins-vm_winston_ws 37.61% <100.00%> (-0.03%) ⬇️
profiling 43.62% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-aws-sdk 33.30% <100.00%> (-0.06%) ⬇️
serverless-aws-sdk-bedrockruntime 32.17% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-client 37.21% <100.00%> (+0.03%) ⬆️
serverless-aws-sdk-dynamodb 34.10% <100.00%> (-0.12%) ⬇️
serverless-aws-sdk-eventbridge 27.25% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-kinesis 37.41% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-lambda 34.60% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-s3 32.60% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-serverless-peer-service 39.47% <100.00%> (-0.16%) ⬇️
serverless-aws-sdk-sns 38.26% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-sqs 38.01% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-stepfunctions 33.19% <100.00%> (-0.07%) ⬇️
serverless-aws-sdk-util 47.95% <ø> (ø)
serverless-bucket-0 39.47% <100.00%> (+<0.01%) ⬆️
serverless-lambda 34.33% <100.00%> (-0.09%) ⬇️
test-optimization-cucumber 52.51% <100.00%> (+0.05%) ⬆️
test-optimization-cypress 49.73% <100.00%> (+0.07%) ⬆️
test-optimization-jest 55.67% <100.00%> (+0.17%) ⬆️
test-optimization-mocha 53.69% <100.00%> (+0.11%) ⬆️
test-optimization-playwright-playwright-atr 43.59% <100.00%> (+0.09%) ⬆️
test-optimization-playwright-playwright-efd 43.87% <100.00%> (+0.09%) ⬆️
test-optimization-playwright-playwright-final-status 43.90% <100.00%> (+0.06%) ⬆️
test-optimization-playwright-playwright-impacted-tests 43.41% <100.00%> (+<0.01%) ⬆️
test-optimization-playwright-playwright-reporting 43.50% <100.00%> (+0.09%) ⬆️
test-optimization-playwright-playwright-test-management 44.93% <100.00%> (+0.09%) ⬆️
test-optimization-playwright-playwright-test-span 44.78% <100.00%> (+0.08%) ⬆️
test-optimization-selenium 45.52% <100.00%> (+0.07%) ⬆️
test-optimization-testopt 48.28% <100.00%> (+0.09%) ⬆️
test-optimization-vitest 50.93% <100.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Overall package size

Self size: 6.39 MB
Deduped: 7.46 MB
No deduping: 7.46 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.2.0 | 104.26 kB | 843.44 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Mar 12, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 90.17%
Overall Coverage: 87.96% (-0.04%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: dcea155 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Mar 12, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-06-26 23:18:26

Comparing candidate commit dcea155 in PR branch crysmags/graphql-migration with baseline commit 1ed6244 in branch master.

📊 Benchmarking dashboard

Found 10 performance improvements and 18 performance regressions! Performance is the same for 2213 metrics, 45 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:plugin-graphql-long-with-depth-and-collapse-off-24

  • 🟩 max_rss_usage [-80.546MB; -29.971MB] or [-15.231%; -5.667%]

scenario:plugin-graphql-long-with-depth-off-20

  • 🟩 instructions [-5.7G instructions; -1.7G instructions] or [-9.335%; -2.755%]

scenario:plugin-graphql-long-with-depth-on-max-24

  • 🟩 cpu_user_time [-507.427ms; -191.188ms] or [-10.472%; -3.945%]
  • 🟩 execution_time [-516.065ms; -194.437ms] or [-10.408%; -3.921%]
  • 🟩 instructions [-2.7G instructions; -1.0G instructions] or [-10.774%; -4.043%]
  • 🟩 throughput [+6.393op/s; +16.993op/s] or [+3.926%; +10.436%]

scenario:plugin-graphql-long-with-depth-on-max-26

  • 🟩 cpu_user_time [-477.688ms; -166.467ms] or [-9.340%; -3.255%]
  • 🟩 execution_time [-479.270ms; -166.010ms] or [-9.142%; -3.167%]
  • 🟩 instructions [-2.2G instructions; -0.8G instructions] or [-8.370%; -3.148%]
  • 🟩 throughput [+4.839op/s; +14.095op/s] or [+3.145%; +9.160%]

scenario:startup-with-tracer-everything-20

  • 🟥 cpu_user_time [+73.727ms; +124.628ms] or [+4.456%; +7.532%]
  • 🟥 execution_time [+75.819ms; +129.806ms] or [+3.963%; +6.785%]
  • 🟥 instructions [+150.1M instructions; +244.1M instructions] or [+2.559%; +4.161%]

scenario:startup-with-tracer-everything-24

  • 🟥 cpu_user_time [+47.340ms; +85.439ms] or [+3.376%; +6.093%]
  • 🟥 execution_time [+53.288ms; +97.668ms] or [+3.149%; +5.771%]
  • 🟥 instructions [+109.4M instructions; +178.9M instructions] or [+2.033%; +3.324%]
  • 🟥 max_rss_usage [+4.690MB; +8.689MB] or [+2.531%; +4.690%]

scenario:startup-with-tracer-everything-26

  • 🟥 cpu_user_time [+50.479ms; +90.288ms] or [+3.789%; +6.777%]
  • 🟥 execution_time [+56.827ms; +100.380ms] or [+3.493%; +6.170%]
  • 🟥 instructions [+115.1M instructions; +188.0M instructions] or [+2.247%; +3.671%]
  • 🟥 max_rss_usage [+5.693MB; +9.304MB] or [+3.011%; +4.921%]

scenario:startup-with-tracer-everything-esm-20

  • 🟥 cpu_user_time [+68.789ms; +130.992ms] or [+3.022%; +5.755%]
  • 🟥 execution_time [+69.097ms; +131.447ms] or [+2.665%; +5.070%]
  • 🟥 instructions [+154.9M instructions; +254.0M instructions] or [+2.307%; +3.785%]

scenario:startup-with-tracer-everything-esm-24

  • 🟥 cpu_user_time [+51.874ms; +97.187ms] or [+3.112%; +5.831%]
  • 🟥 execution_time [+53.355ms; +101.115ms] or [+2.697%; +5.111%]

scenario:startup-with-tracer-everything-esm-26

  • 🟥 cpu_user_time [+45.723ms; +98.187ms] or [+2.833%; +6.083%]
  • 🟥 execution_time [+47.040ms; +101.115ms] or [+2.433%; +5.231%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:appsec-appsec-enabled-24

  • unstable execution_time [-179.110ms; +183.342ms] or [-6.825%; +6.986%]

scenario:appsec-appsec-enabled-26

  • unstable execution_time [-213705.016µs; +215495.182µs] or [-8.491%; +8.562%]

scenario:appsec-appsec-enabled-with-attacks-26

  • unstable execution_time [-157.152ms; +154.329ms] or [-5.464%; +5.366%]

scenario:appsec-control-20

  • unstable execution_time [-157.739ms; +151.330ms] or [-9.033%; +8.666%]

scenario:appsec-control-24

  • unstable execution_time [-104.086ms; +106.097ms] or [-8.588%; +8.754%]

scenario:appsec-control-26

  • unstable execution_time [-110327.955µs; +110230.288µs] or [-9.153%; +9.145%]

scenario:debugger-line-probe-with-snapshot-default-26

  • unstable cpu_user_time [-3755.746ms; +2527.326ms] or [-35.795%; +24.087%]
  • unstable execution_time [-3773.340ms; +2554.867ms] or [-33.689%; +22.810%]
  • unstable instructions [-33.2G instructions; +22.3G instructions] or [-37.746%; +25.285%]
  • unstable max_rss_usage [-10.544MB; +6.606MB] or [-6.416%; +4.020%]
  • unstable throughput [-565.225op/s; +836.064op/s] or [-18.620%; +27.543%]

scenario:debugger-line-probe-with-snapshot-minimal-24

  • unstable cpu_user_time [-2625.726ms; +3793.423ms] or [-29.031%; +41.941%]
  • unstable execution_time [-2664.310ms; +3852.698ms] or [-27.392%; +39.609%]
  • unstable instructions [-21.8G instructions; +31.1G instructions] or [-29.594%; +42.267%]
  • unstable max_rss_usage [-8.861MB; +13.391MB] or [-5.460%; +8.251%]
  • unstable throughput [-1123.740op/s; +784.402op/s] or [-32.019%; +22.350%]

scenario:debugger-line-probe-with-snapshot-minimal-26

  • unstable cpu_user_time [-2049.426ms; +3470.799ms] or [-22.206%; +37.607%]
  • unstable execution_time [-2060.236ms; +3506.540ms] or [-20.792%; +35.389%]
  • unstable instructions [-18.0G instructions; +30.7G instructions] or [-23.491%; +40.025%]
  • unstable throughput [-788.715op/s; +437.099op/s] or [-23.707%; +13.138%]

scenario:debugger-line-probe-without-snapshot-24

  • unstable cpu_user_time [-3286.027ms; +2152.040ms] or [-36.520%; +23.917%]
  • unstable execution_time [-3364.857ms; +2204.859ms] or [-34.717%; +22.749%]
  • unstable instructions [-27.9G instructions; +18.4G instructions] or [-37.825%; +24.950%]
  • unstable max_rss_usage [-11.853MB; +7.554MB] or [-7.322%; +4.666%]
  • unstable throughput [-657.904op/s; +996.268op/s] or [-18.672%; +28.276%]

scenario:debugger-line-probe-without-snapshot-26

  • unstable cpu_user_time [-2093.818ms; +3327.931ms] or [-22.431%; +35.651%]
  • unstable execution_time [-2076.376ms; +3325.821ms] or [-20.709%; +33.171%]
  • unstable instructions [-18.7G instructions; +29.8G instructions] or [-24.150%; +38.442%]
  • unstable max_rss_usage [-6.429MB; +10.132MB] or [-4.044%; +6.373%]
  • unstable throughput [-732.697op/s; +455.478op/s] or [-22.364%; +13.903%]

scenario:dogstatsd-with-tags-20

  • unstable cpu_user_time [-439.642ms; +275.772ms] or [-9.157%; +5.744%]
  • unstable execution_time [-441.750ms; +276.094ms] or [-9.064%; +5.665%]
  • unstable throughput [-94311.261op/s; +151776.791op/s] or [-5.469%; +8.801%]

scenario:plugin-graphql-long-with-depth-and-collapse-off-20

  • unstable max_rss_usage [-72.285MB; -26.130MB] or [-17.438%; -6.304%]

scenario:plugin-graphql-long-with-depth-and-collapse-off-26

  • unstable max_rss_usage [-78.222MB; -29.417MB] or [-16.522%; -6.213%]

scenario:plugin-graphql-long-with-depth-off-20

  • unstable cpu_user_time [-1336.206ms; -175.223ms] or [-12.600%; -1.652%]
  • unstable execution_time [-1356.253ms; -181.922ms] or [-12.616%; -1.692%]
  • unstable throughput [+1.572op/s; +13.816op/s] or [+1.458%; +12.808%]

scenario:plugin-graphql-long-with-depth-off-26

  • unstable max_rss_usage [+15.008MB; +51.198MB] or [+7.869%; +26.845%]

scenario:plugin-graphql-long-with-depth-on-max-20

  • unstable cpu_user_time [-1.885s; -0.559s] or [-18.294%; -5.420%]
  • unstable execution_time [-1.866s; -0.553s] or [-17.757%; -5.261%]
  • unstable instructions [-8.7G instructions; -3.3G instructions] or [-17.285%; -6.544%]
  • unstable max_rss_usage [-21.898MB; +41.233MB] or [-12.329%; +23.215%]
  • unstable throughput [+3.836op/s; +13.935op/s] or [+4.952%; +17.987%]

scenario:test-optimization-large-suite-20

  • unstable max_rss_usage [-4.425MB; +7.638MB] or [-5.463%; +9.430%]

@crysmags crysmags changed the title This is a draft made with AIT - feat(graphql): migrate shimmer to orchestrion instrumentation feat(graphql): migrate shimmer to orchestrion instrumentation Apr 3, 2026
@crysmags
crysmags force-pushed the crysmags/graphql-migration branch 2 times, most recently from 28daea8 to 297e8ac Compare April 3, 2026 21:59
@crysmags
crysmags marked this pull request as ready for review April 3, 2026 22:13
@crysmags
crysmags requested review from a team as code owners April 3, 2026 22:13
@crysmags crysmags added the ai-generated PR created with AI assistance label Apr 6, 2026
@crysmags
crysmags force-pushed the crysmags/graphql-migration branch from 297e8ac to 36022cf Compare April 20, 2026 21:30
@crysmags

crysmags commented Apr 23, 2026

Copy link
Copy Markdown
Collaborator Author

Benchmark update (revised — supersedes the original analysis posted above)

Since the original comment landed, three things changed:

  1. The companion plugin-graphql-long bench merged via bench: add plugin-graphql-long sirun benchmark #8089 and is now in master, so the comparison runs side-by-side with the existing 6-query bench on every PR.
  2. A new with-tracer-graphql-disabled variant was added in this PR (commit 3) — runs tracer.init() + tracer.use('graphql', false) so it captures dd-trace's shared startup cost without graphql plugin work. Comparing other tracer-on variants against this one isolates plugin-specific overhead.
  3. Master picked up 4774cc13a perf(graphql): memoize the apollo signature pipeline between iterations, and PR 7757's getSignature benefits from it via the rebase.

Current numbers (PR head vs master, run 2026-05-26)

variant direction reading
with-depth-on-max 🟩 −15 to −34% cpu/execution (Node 20/22/24) typical production config — large per-resolver wins
with-depth-and-collapse-on 🟩 −14 to −35% cpu/execution (Node 20/22/24) full instrumentation, collapsed — large wins
with-depth-and-collapse-off 🟩 max_rss −5 to −9% (Node 22/24) per-field spans mode — memory wins, cpu ~tied
with-tracer-graphql-disabled 🟨 tied shared dd-trace startup cost is equivalent between branches
with-depth-off 🟥 +5–8% cpu/execution on Node 20 structural cost — see below

The depth=0 residual

with-depth-off (tracer.use('graphql', { depth: 0 })) shows +5–8% on Node 20. Investigated thoroughly; the cost traces to orchestrion's publish format carrying raw function arguments. Master's pre-orchestrion shimmer wrapper pre-computed { operation, args, docSource } before publishing, so master's bindStart got them for free. The orchestrion plugin's bindStart re-extracts them per call. For depth=0 (no resolve spans created), that per-execute extraction is the only thing happening, so it dominates the variant.

Local subtraction confirms it:

  • master depth=0 ≈ master tracer-graphql-disabled (master's resolve plugin self-disables on depth=0 and gets pre-computed payload → near-zero per-execute work)
  • PR 7757 depth=0 = PR tracer-graphql-disabled +130ms per process

That ~130ms is the orchestrion-vs-shimmer publish-format cost on the depth=0 specific path. Behavior is unchanged from master: depth=0 still emits graphql.execute spans and skips graphql.resolve spans.

Closing it requires either changing depth=0's semantics (breaking change for users) or changing orchestrion's publish payload to carry pre-computed values (out of scope for this PR, would benefit every plugin migrating to orchestrion).

Separate concern: startup-with-tracer-everything

startup-with-tracer-everything (a different bench, not graphql-specific) shows +9–14% cpu and +8–12% execution_time on tracer-init cost when every integration's hooks register. Most likely orchestrion's AST rewriting of graphql at module load. Worth flagging — this is whole-tracer startup, not graphql query time. Possibly a separate follow-up for orchestrion-side caching.

Net read

Real production graphql traffic runs at depth>0 and gets the large wins. The single configuration that regresses (depth=0) is an explicit "resolver tracing off" mode users don't typically run at scale, and the +130ms per process is bounded — it doesn't scale with workload size.

@crysmags
crysmags force-pushed the crysmags/graphql-migration branch from fe1544c to 38b97c3 Compare May 1, 2026 15:36
@crysmags
crysmags force-pushed the crysmags/graphql-migration branch 3 times, most recently from 9696d95 to b83c40b Compare May 20, 2026 13:44
@crysmags
crysmags force-pushed the crysmags/graphql-migration branch from 34f8007 to 131cbcd Compare June 3, 2026 19:30
crysmags and others added 4 commits June 3, 2026 16:17
Replaces the shimmer-based graphql instrumentation in
packages/datadog-instrumentations/src/graphql.js with orchestrion AST
rewriting for the top-level execute / parse / validate entry points
(both CJS and ESM in graphql >= 0.10 and @graphql-tools/executor).

Plugin architecture changes:
  - packages/datadog-plugin-graphql/src/execute.js: takes over resolver
    wrapping that used to live in instrumentations; uses a single
    rootCtx per execute() call to coordinate execute-span creation and
    per-resolver bookkeeping. Resolve spans are recorded inline at
    first-encounter and finished from the resolver callback so encoder
    state stays hot across span finalization.
  - packages/datadog-plugin-graphql/src/resolve.js: deleted. The
    resolve-plugin sub-class was needed only because the old shimmer
    architecture published per-field events through a separate
    plugin; the orchestrion-based design folds that work into the
    execute plugin and uses a module-level _depthDisabled flag to
    short-circuit when tracer.use('graphql', { depth: 0 }).
  - packages/datadog-plugin-graphql/src/parse.js,validate.js: hooked
    via orchestrion publishes; documentSources mapping moved onto the
    parse plugin so the execute plugin can read raw query source for
    graphql.source tags + IAST taint tracking.

Correctness fixes wired up alongside the migration:
  - IAST taint tracking: orchestrion's bindStart now fires the same
    apm:graphql:resolve:start payload the old shimmer wrapper produced,
    using the actual args object graphql constructs internally rather
    than an AST-rebuilt copy. Restores taint propagation for hardcoded
    literal resolver arguments (e.g. books(title: "ls")) which the
    initial orchestrion port broke.
  - AppSec resolver blocking: passes the abort controller directly to
    the channel payload so AppSec subscribers can abort the resolver
    synchronously. packages/dd-trace/src/appsec/graphql.js updated to
    read req via getActiveRequest() (the storage('legacy') call only
    worked under the old shimmer setup).
  - Channel payload shape change in packages/dd-trace/src/appsec/
    channels.js to match the new bindStart contract.

Performance work folded in (per-execute and per-resolver):
  - Lazy-build info object and resolver args; only construct when the
    plugin actually needs them.
  - Eliminate per-field channel publishing — the per-execute rootCtx
    holds field records and we publish nothing per resolver in the
    APM-only path.
  - Collapse redundant path walks in the resolve bindStart so
    pathToArray runs once per resolver call.
  - Consolidate per-field stash + finish handler into one shared
    object; avoids reallocating ctx objects per resolver.
  - Short-circuit re-entrant execute() on the same contextValue
    (covers yoga's normalizedExecutor pattern that would otherwise
    double-span).
  - Hot fast-path when depth=0: cache the disabled state on a
    module-level flag in configure() so resolveAsync bails on a single
    property read, matching master's startResolveCh.hasSubscribers
    cost.
  - Port of #8309's signature-pipeline optimization onto the migrated
    execute plugin.

All 231 plugin-graphql tests pass.

Local sirun on plugin-graphql-long (100q × 5 iters per Node version)
shows per-process user-CPU vs master:
  with-depth-on-max:                   -340 ms  (-31%)
  with-depth-and-collapse-off:           tied
  with-tracer-graphql-disabled:         -50 ms  (-7%)
  with-depth-off:                       +130 ms (+21%)

The depth=0 regression is the only configuration where PR is slower
than master. It traces to orchestrion's publish format carrying raw
function args (master's shimmer wrapper pre-computed
{operation, args, docSource} before publishing). For variants with any
per-resolver work, PR's structural wins overtake this cost many times
over.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adds the public TypeScript declaration for the new resolve hook and the
field context object it receives, surfaced as part of the orchestrion
migration. The runtime invocation lives in the execute plugin
(see prior commit); this commit declares the user-facing contract.

```ts
interface FieldContext {
  fieldName: string
  path: string
  error: Error | null
  result: unknown
}

interface plugins.graphql.hooks {
  resolve?: (span?: Span, field?: FieldContext) => void
  ...
}
```

Hook fires once per resolved field (post-resolve, before span.finish),
giving users a hook to add custom tags to graphql.resolve spans or
inspect the resolver result without subscribing directly to the
diagnostic channel.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…t + microbench harness

Two additions to the plugin-graphql-long sirun bench:

1. New variant `with-tracer-graphql-disabled`: runs `tracer.init() +
   tracer.use('graphql', false)`. dd-trace is fully loaded but the
   graphql plugin is explicitly off. The variant is equivalent on master
   and on PR 7757 (both honor the explicit-false), so comparing the
   other tracer-on variants against this one lets a reviewer subtract
   shared dd-trace startup cost from variant deltas to isolate
   graphql-plugin-specific overhead.

2. `microbench.js`: standalone microbenchmarks for the graphql plugin's
   hot-path functions (wrapResolve, assertField, channel publish costs,
   schema-walk wrap setup). Lets contributors pinpoint per-function
   regressions during plugin work without going through the full sirun
   matrix. Run with `node benchmark/sirun/plugin-graphql-long/microbench.js`.
   Modes: PURE=1 (no tracer), NOSUB=1 (tracer loaded + plugin disabled),
   default (full tracing on). Not picked up by CI — sirun only runs
   meta.json variants.

Together these provide both a baseline subtraction in CI and a faster
local feedback loop when iterating on the plugin.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…th,FieldCtx> optimization

Test fix: agent.load() between mocha suites replaces globalThis[dd-trace] with a new
object, but graphql.js's module-level ddGlobal capture still pointed to the old one.
IITM hook for execution/execute.js doesn't re-fire (module cached), so
graphql_defaultFieldResolver was never set on the new ddGlobal. Replaced with a local
defaultFieldResolver function in execute.js that mirrors graphql's behavior and survives
any number of agent.load() reloads. Also removed the dedicated execution/execute.js
addHook that existed only to capture defaultFieldResolver; fixed printer/visitor/utilities
hooks to read ddGlobal lazily instead of at module load time.

Perf (BridgeAR a39f889 port): fields: Object.create(null) -> new Map(); non-collapse
keyed by path node identity (O(1), no string construction per repeated resolver call);
shouldInstrumentNode operates on linked-list before Map lookup; path strings computed
lazily on first-encounter only; getParentField walks path.prev chain for non-collapse;
full GraphQLResolveInfo not retained per field - pluck fieldNode/fieldName/returnType.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@crysmags
crysmags force-pushed the crysmags/graphql-migration branch from 131cbcd to e216b9f Compare June 3, 2026 20:18
…s objects

setWrappedFieldResolver was unconditionally writing fieldResolver onto
rawArgs[0], which throws TypeError when the caller passes a frozen args
object and silently double-wraps caller-supplied resolvers on reuse.

- Skip mutation entirely when Object.isExtensible returns false
- Skip when caller already has own fieldResolver property
- Same contract for positional-call path (rawArgs[6])

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
…imitives

The previous `normalizeContextValue` helper mutated the caller's execute args
to force `contextValue` to be an object (so it could key the contexts WeakMap),
substituting `{}` for any falsy/primitive value. Two regressions ride on that:

  - resolvers received a synthetic `{}` instead of what the caller passed, so
    user code observed a different contextValue with the tracer enabled than
    without — `forwards undefined to resolvers (positional form)` and its
    seven sibling tests (false/0/string/null/42/'request-1'/Symbol) caught
    this on master in #8502;
  - truthy primitives (`42`, `'request-1'`, `Symbol()`) bypassed the
    substitution and crashed at `contexts.set(contextValue, ctx)` with
    `TypeError: Invalid value used as weak map key`, killing the test runner
    process partway through the suite.

Drop the mutation. Use the caller's contextValue verbatim. Guard the WeakMap
write with `isWeakMapKey`; for non-keyable values, store rootCtx in a module-
level AsyncLocalStorage so `resolveAsync` can still find it via
`primitiveContextAls.getStore()` when the WeakMap lookup misses.

Also pre-compute `baseTypeName` (List/NonNull-unwrapped type name) at field-
record creation instead of reading `returnType?.name` at span-tag time —
master #8502's fix for the field-type-tag bug, ported here so resolver spans
for wrapped types report the underlying name instead of `undefined`.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
crysmags and others added 4 commits June 9, 2026 11:00
…e-wrap guard

@BridgeAR's nits on packages/datadog-instrumentations/src/graphql.js:

  - Replace the hand-rolled `patchExecuteExport` (writable-vs-getter
    branch via try/catch + Object.defineProperty) with `shimmer.wrap`.
    shimmer handles both shapes via Object.getOwnPropertyDescriptor and
    the descriptor.writable/.configurable/.enumerable branches in
    packages/datadog-shimmer/src/shimmer.js (~L130-200).
  - Replace the `wrapped.__dd_wrapped = true` boolean property with a
    module-level `WeakSet` tracking already-wrapped function instances.
    The set is checked at the top of `wrapExecute`; idempotent across
    repeated module-load hook fires without polluting the function with
    a tracer-specific property.
  - Drop the conditional object-form narrowing of arguments[0]. The
    receiver (AppSec subscriber) is responsible for deciding what to do
    with the value it sees — passing through whatever the caller
    actually invoked execute() with is cleaner and matches the master
    pattern.
  - Trim the wall-of-text comments. Kept only the one paragraph
    explaining *why* the wrap exists outside the orchestrion try-block
    (dc-polyfill catches subscriber throws), since that's the
    non-obvious bit.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
…nch rework

The earlier revert (90a60ba) used a stale local master ref and pulled an
older version of these files. Master has since picked up #8787 (profiler
workload + llmobs guard + shortened benches) and #8734 (integration tuning)
which both touch these paths, so the branch went CONFLICTING and Actions
stopped triggering on new commits.

Pulls in origin/master's current shape for both meta.json (3 variants,
adjusted query counts) and index.js (startup guard + assert + the
no-WITH_ASYNC_HOOKS path). No graphql plugin code is affected.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
…y under the startup-guard ceiling

The orchestrion-migrated resolveAsync trims a handful of cycles per call
relative to master's shimmer path — `_getTime()` unconditional (no ternary),
single-pass template-literal path-build over the graphql Path linked list,
dropped result-truthy precheck before the thenable test. Net effect at
depth=4 + collapse=on is the 800-query loop now finishes fast enough that
setup + module load + tracer init push past the 10% startup-share ceiling
the bench asserts at the bottom of the loop, tripping:

  AssertionError: startup-guard: load+setup was 10.0% of the run (max 10%)
  at /app/candidate/benchmark/sirun/startup-guard.js:44

(Reported on dd-gitlab/benchmark: [24, 4], wall time 14.263s for the 800-
query variant on this branch.)

Bump the variant's QUERIES to 1100 — same value `with-depth-off` already
uses — so the loop dominates total runtime by a comfortable margin again.
Other variants (`with-depth-off` at 1100, `with-depth-and-collapse-off`
at 200) keep their existing calibration and pass.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
…ide bindStart

The previous design wrapped graphql.execute twice — once with shimmer
(for the apm:graphql:execute:start synchronous-abort contract) and again
with the orchestrion-emitted tracingChannel runtime. The earlier analysis
stopped at "channel.publish and bindStore transforms can't propagate
throws" and reached for shimmer as the only mechanism that could surface
AbortError to graphql.execute's caller.

That missed the orchestrion-emitted wrapper's own propagation path. The
rewriter generates (captured from a live run, graphql 16.12.0):

  function execute(__apm$arg0, ...__apm$args) {
    const __apm$arguments = [__apm$arg0, ...__apm$args].slice(0, arguments.length);
    const __apm$ctx = { arguments: __apm$arguments, self: this, ... };
    const __apm$traced = () => {
      const __apm$wrapped = function (args) {
        const { schema, document, ... } = args;     // <- destructures immediately
        // ...
      };
      return __apm$wrapped.apply(this, __apm$arguments);
    };
    if (!tr_ch_apm_hasSubscribers(ch)) return __apm$traced();
    return ch.start.runStores(__apm$ctx, () => {
      try {
        const result = __apm$traced();
        __apm$ctx.result = result;
        return result;
      } catch (err) {
        __apm$ctx.error = err;
        ch.error.publish(__apm$ctx);
        throw err;                                  // <- THIS rethrows fn's error
      } finally {
        ch.end.publish(__apm$ctx);
      }
    });
  }

The catch+rethrow propagates errors thrown by __apm$traced (the wrapped
fn) synchronously to graphql.execute's caller. Subscriber and transform
throws are caught and queued via process.nextTick → triggerUncaughtException
(confirmed in Node 25's lib/diagnostics_channel.js source), but a throw
from the wrapped fn itself escapes cleanly.

The bindStart transform receives __apm$ctx, and __apm$ctx.arguments is
the SAME array reference that __apm$wrapped.apply spreads at call time.
Mutations to ctx.arguments[0] from inside bindStart are visible to
graphql's body when it runs.

So the orchestrion-pure abort gate:

  1. bindStart synchronously publishes apm:graphql:execute:start with
     { abortController, args }. AppSec/WAF subscribers run inline.
  2. After publish returns, bindStart inspects abortController.signal.aborted.
  3. If aborted, bindStart replaces ctx.arguments[0] with a Proxy whose
     get/has traps throw AbortError, sets ctx.ddAborted = true, and
     returns ctx.currentStore (skipping resolver-wrapping work since no
     resolvers will run anyway).
  4. The wrapper then calls __apm$wrapped(ctx.arguments[0]); graphql's
     first statement `const { schema, ... } = args` triggers the Proxy
     trap → throws AbortError → the wrapper's catch catches → rethrows
     → graphql.execute's caller catches it via assert.throws.

The plugin's error(ctx) checks ctx.ddAborted and skips the error tag
(per the contract, opSpan.error stays 0 for the abort case). end(ctx)
sees ctx.error and #drain()s the span clean.

Drops:
  - packages/datadog-instrumentations/src/graphql.js: removes the
    shimmer wrap, the wrappedExecutes WeakSet, the AbortError class,
    the patchExecuteExport function, the apm:graphql:execute:start
    publish, and the datadog:graphql:execute:abort internal channel.
    File is back to a thin orchestrion-only stub plus the ddGlobal
    capture hooks.
  - packages/datadog-plugin-graphql/src/execute.js: removes the
    `addSub(abortExecuteCh.name, ...)` subscription and the
    #handleAbort method (no longer needed — bindStart handles span
    creation and the wrapper handles the AbortError propagation).
    Adds the apm:graphql:execute:start publish + abort gate inline in
    bindStart.

Wins:
  - One wrap layer instead of two. Closes the per-execute perf delta
    the bench dashboard was showing vs master.
  - PR title "feat(graphql): migrate shimmer to orchestrion
    instrumentation" is now accurate.
  - Aligns with the wider tracer's move toward pure orchestrion
    instrumentation.

Verified: all 4 originally-failing tests (the abort pair, updateField
sibling-finish, forwards-undefined-positional) pass; the no-mutation
guard tests (`should not overwrite the caller-supplied fieldResolver`,
`should not add fieldResolver to a frozen caller-owned execute args
object`) pass; full sweep on graphql 0.10.0 + 16.x + 15.2.0 shows
238 passing 0 failing locally.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has done a huge leap and it is almost ready in my opinion. The depthDisabled still needs addressing and a few smaller comments would polish it a bit before landing :)

Comment thread benchmark/sirun/plugin-graphql-long/meta.json Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/index.js
Comment thread packages/datadog-plugin-graphql/src/parse.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js
crysmags and others added 3 commits June 10, 2026 10:34
…ddress review nits

Targets the per-resolver overhead that's been driving the +7-15%
instructions delta vs master on with-depth-on-max across Node 20/24/26.

execute.js — resolveAsync
  - Combined fast-path at the top: when depth=0 AND neither IAST nor
    AppSec has a subscriber, skip everything (rootCtx lookup, path walk,
    publishes, span machinery). This is the common production case.
  - pathString is computed exactly ONCE per resolver call, collapse-
    aware (`buildCollapsedPathStringFromNode` when collapse, else
    `buildPathStringFromNode`). The IAST publish and the field record
    share the same computed value — previously the field-record path
    recomputed it inside the `isFirst` branch and the collapse branch
    recomputed `collapsedKey` separately.
  - Same for `collapsedKey`: assigned once, reused for `mapKey` and the
    field record.
  - depth gate is now a single combined check
    `if (depthDisabled || !shouldInstrumentNode(...))` after publishes,
    not two separate checks.

execute.js — other
  - Drop the duplicate local `getSignature` — it already lives in
    `./utils.js` and exports cleanly. Import from there.
  - `getBaseTypeName` rename `t` → `cursor`, add a defensive null guard
    on the loop condition (`while (cursor && cursor.ofType)`).
  - `getParentField` inner shadow: rename the looked-up `field` to
    `innerField` so it doesn't shadow the function parameter.
  - `setWrappedFieldResolver` now returns the (possibly cloned) args
    view directly; `bindStart` assigns `ctx.ddArgs = setWrappedFieldResolver(...)`
    in one step instead of the previous `setWrappedFieldResolver(...);
    ctx.ddArgs = readArgs(ctx.arguments)` pair. Saves one function call +
    one property lookup per execute.

parse.js
  - Hoist the `if (document)` outer so the documentSources logic is one
    branch: read the cached source for an already-known document, or
    cache the new (document, source) mapping when both are present.

index.js
  - Add a one-line comment explaining why `resolve` has no noop fallback
    — it runs per-field (hot path), and `finishResolveSpan` gates with
    `if (this.config.hooks.resolve)` so absent-hook callers skip both
    the call and the per-field payload-object allocation.

benchmark/sirun/plugin-graphql-long/meta.json
  - Bump `with-depth-on-max` QUERIES 1100 → 1500 per @BridgeAR's note
    that the operations are faster now; gives the startup-guard ratio
    more margin and the bench more samples per partition.

.agents/skills/apm-integrations/references/orchestrion.md
  - New section "Propagating Synchronous Errors From bindStart" — captures
    the Proxy-on-arguments pattern we used for the
    `apm:graphql:execute:start` WAF abort contract. The wrapped fn's
    destructure triggers the Proxy trap, the orchestrion wrapper's
    catch+rethrow propagates the AbortError synchronously to the caller.
    Documents the constraint (subscriber/transform throws are swallowed
    by Node's diagnostics_channel), the contract, and the
    `error()`/`end()` shape that keeps `opSpan.error === 0` on the abort
    path.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
The 1100 → 1500 bumps were defending against the startup-guard 10%
threshold firing on our branch (the orchestrion-pure path runs the loop
faster per-iteration, so setup ratio creeps higher). But the bench
dashboard compares raw total instruction counts vs master's baseline at
QUERIES=800 — running ~2x more queries on our branch inflates the
total proportionally and reads as a +55-60% regression even though
per-query we're ~17% faster than master.

Math on the +55.4% Node 24 number:
  ours_total / master_total = 1.554
  ours_total = 1500 × ours_per_query, master_total = 800 × master_per_query
  → ours_per_query / master_per_query = 1.554 × 800 / 1500 = 0.829

We do ~17% fewer instructions per query on Node 24/26; Node 20 is
near-parity. The dashboard "regression" is QUERIES inflation.

Revert to master's 800. If startup-guard fires from our faster per-query
loop, we'll investigate the bench framework rather than papering over
with more queries.

Co-Authored-By: Claude Opus 4.7 <[email protected]>

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving closer :D

Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
crysmags and others added 4 commits June 22, 2026 12:48
Addresses BridgeAR's review on the orchestrion execute plugin:

- Primitive/non-keyable contextValue no longer uses
  `primitiveContextAls.enterWith()` — that sets the ALS store on the
  caller's async frame and can leak past execute. Carry rootCtx on the
  orchestrion-scoped store (`ctx.currentStore.graphqlRootCtx`) that
  `runStores` already enters for execute instead; resolvers read it via
  `legacyStorage.getStore()` and it unwinds with the frame. Drops the
  AsyncLocalStorage import entirely.

- Restore the per-node pathString cache (`rootCtx.pathCache`) that master
  had. The branch was re-walking the whole path linked-list on every
  resolver call (O(depth) per call); the cache builds each pathString
  incrementally off the parent's cached value (O(1) amortized).
  Microbench (deeply nested resolvers): ~3.9x at depth 30, ~6.7x at
  depth 60, ~parity shallow. Reproducible across fresh processes.

- Guard the pathString build on `infoPath` so a missing `info.path`
  doesn't throw in either the collapse or non-collapse branch
  (`pathToArray`/`shouldInstrumentNode` already tolerate undefined).

- Extract the duplicated object-form arg detection shared by `readArgs`
  and `setWrappedFieldResolver` into one `isObjectForm` helper.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated
Comment thread packages/datadog-plugin-graphql/src/execute.js Outdated

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a major undertaking!

Rewriting this instrumentation is definitely a very hard task!

I believe this is now in a very good state! Thank you for the hard work!

LGTM

@crysmags
crysmags merged commit 27dcc31 into master Jun 27, 2026
805 checks passed
@crysmags
crysmags deleted the crysmags/graphql-migration branch June 27, 2026 03:06
dd-octo-sts Bot pushed a commit that referenced this pull request Jun 29, 2026
)

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 <[email protected]>
  Co-authored-by: Ruben Bridgewater <[email protected]>
This was referenced Jun 29, 2026
rochdev pushed a commit that referenced this pull request Jul 2, 2026
)

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 <[email protected]>
  Co-authored-by: Ruben Bridgewater <[email protected]>
rochdev pushed a commit that referenced this pull request Jul 2, 2026
)

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 <[email protected]>
  Co-authored-by: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-generated PR created with AI assistance semver-patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants