fix(graphql)!: decouple resolver depth from path collapsing#8774
Conversation
Overall package sizeSelf size: 6.4 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 |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ed48c46 | Docs | Datadog PR Page | Give us feedback! |
BridgeAR
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24d24f2af6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8774 +/- ##
==========================================
- Coverage 93.69% 93.68% -0.01%
==========================================
Files 889 889
Lines 50856 50858 +2
Branches 11830 11831 +1
==========================================
+ Hits 47647 47648 +1
- Misses 3209 3210 +1 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-06-27 13:15:11 Comparing candidate commit ed48c46 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2255 metrics, 31 unstable metrics.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaccfa06b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
rochdev
left a comment
rochdev
left a comment
There was a problem hiding this comment.
This is working as intended already. Let's document this better instead of adding even more options which could exacerbate confusion.
IMO the functionality is quite confusing as is. It is a technical aspect how it currently works without being obviously related to how the query is written. As a user I would strongly favor the change (it also seems evident that users do want this, since there are issues asking for it and people opening PRs to implement it like that). |
eaccfa0 to
b8a5a6d
Compare
The `depth` filter counted a resolver's full execution path, including the numeric list indices that `collapse` later folds away. The same query therefore reached a different depth depending on whether `collapse` was on: a field one list-hop below the limit was instrumented when collapsing was off and dropped when it was on, even though both describe the same selection-set nesting. Count only selection-set segments (string path keys) toward `depth`, so the limit tracks query structure rather than execution artifacts. This shifts which resolvers are instrumented at a given `depth`, so it is gated behind `DD_MAJOR`: the v5 line keeps the old list-index counting when collapsing is on, and v6 counts selection-set depth only. The `countListIndices` config flag carries the gate so `shouldInstrumentNode` stays free of version checks. Fixes: #7468
The depth/collapse decoupling shifts which resolvers are instrumented at a given `depth` on the v6 line. Record it in MIGRATING.md, and note on the `depth` type that the limit counts selection-set nesting only, independent of `collapse`. Refs: #7468
b8a5a6d to
7fa59b5
Compare
The with-depth-on-max variant ran at depth 4. The bench query reaches selection-set depth 4 but all-nodes depth 6 (the friends and pets lists each add a numeric index hop). At depth 4 the baseline commit, which counts list indices, instruments ~101 resolvers while the candidate, which counts only selection-set segments, instruments all ~1701 — so the A/B compared different instrumented sets and reported the extra spans as a regression. Depth 6 clears the all-nodes maximum, so both counting modes instrument the identical full set and the variant measures per-resolver overhead rather than a changed workload.
rochdev
left a comment
rochdev
left a comment
There was a problem hiding this comment.
Just realized there was a bug and the expected behaviour was actually not what was going on and this PR fixes this, it doesn't try to change it, so LGTM.
The `depth` filter counted a resolver's full execution path, including the numeric list indices that `collapse` later folds away. The same query therefore reached a different depth depending on whether `collapse` was on: a field one list-hop below the limit was instrumented when collapsing was off and dropped when it was on, even though both describe the same selection-set nesting. Count only selection-set segments (string path keys) toward `depth`, so the limit tracks query structure rather than execution artifacts. This shifts which resolvers are instrumented at a given `depth`, so it is gated behind `DD_MAJOR`: the v5 line keeps the old list-index counting when collapsing is on, and v6 counts selection-set depth only. The `countListIndices` config flag carries the gate so `shouldInstrumentNode` stays free of version checks. Fixes: #7468
The `depth` filter counted a resolver's full execution path, including the numeric list indices that `collapse` later folds away. The same query therefore reached a different depth depending on whether `collapse` was on: a field one list-hop below the limit was instrumented when collapsing was off and dropped when it was on, even though both describe the same selection-set nesting. Count only selection-set segments (string path keys) toward `depth`, so the limit tracks query structure rather than execution artifacts. This shifts which resolvers are instrumented at a given `depth`, so it is gated behind `DD_MAJOR`: the v5 line keeps the old list-index counting when collapsing is on, and v6 counts selection-set depth only. The `countListIndices` config flag carries the gate so `shouldInstrumentNode` stays free of version checks. Fixes: #7468
3 participants
What
The GraphQL resolver
depthfilter counted the full execution path, includingthe numeric list indices that
collapselater folds away. The same query reacheda different depth depending on whether
collapsewas on, so a field one list-hopbelow the limit was instrumented with collapsing off and dropped with it on, even
though both describe the same selection-set nesting.
This counts only selection-set segments (string path keys) toward
depth, so thelimit tracks query structure rather than execution artifacts.
Why gated behind
DD_MAJORIt shifts which resolvers are instrumented at a given
depth, so it ships behindDD_MAJOR: the v5 line keeps the old list-index counting when collapsing is on;v6 counts selection-set depth only. A
countListIndicesconfig flag carries thegate so
shouldInstrumentstays free of version checks.Test plan
test/index.spec.js, gated onDD_MAJOR.test/config.spec.jsproxyquires bothDD_MAJORarms to pin the gate and thedepth count on either release line.
nycconfirms the changed lines/branches are covered; eslint clean.Fixes #7468