Open
Conversation
…mention it in CONTRIBUTING.md
* pass only `AbortSignal`, not `AbortController` * throw the abort `.reason`, not a custom `AbortError` * `cb` is always passed, never falsy * do not call `cb` again if `cb(null, result)` threw an error
…annel not the entire `rootCtx`
Avoid allocating unnecessary object and copying property values back and forth.
* make `getParentField` look up `path.prev` in a `Map`, instead of relying on `pathToArray`. * simplify `getPath`: change higher-order `withCollapse` function into `collapsePath` * remove duplicated `pathToArray` code and call from wrapped `resolve` function
…esolve:start` channel
… `apm:graphql:resolve:finish` channel … to avoid growing `ctx.fields` to a considerable size even when not necessary. Also rename `fieldCtx` to just `field`.
* compute depth from depth of parent field (with complications due to DataDog#7468) * check field depth against `config.depth` before even calling `pathToArray` and `collapsePath`, avoiding array allocations * hoist `config.depth < 0` check out of the resolver instrumentation, memoise it on the plugin instance
* instead of ignoring subsequent resolver calls, use a shared context object for them * add the (single) tracer span to this context, and re-enter it * optimise finding existing shared context by looking up `fieldName` in `parentField.shared.collapsedChildren` (or in `rootCtx.toplevelCollapsed` for top-level fields), not by constructing a `computedPathString` (avoiding unnecessary `pathToArray(info.path)` calls)
* get rid of `apm:graphql:resolve:updateField` channel * use `apm:graphql:resolve:finish` channel in normal purpose, publishing to it after every `resolve()` execution * adjust `GraphQLResolvePlugin` methods * add optional `.finalize` hook to resolver field contexts instead * use normal `GraphQLResolvePlugin.error` method for handling non-collapsed fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a more or less complete rewrite of the
GraphQLResolvePlugin. Main features:graphql.hooks.resolveto allow messing with thegraphql.resolvespans. This might e.g. solve [BUG]: Trace is marked as error even despite tagging trace as error false #5226, and is also useful for some traces we collect at $work.apm:graphql:resolve:updateFieldchannel by the more customaryapm:graphql:resolve:finishFieldand not deferring the finishing of resolver spans untilexecutefinishesWeakMapto find their parent), by not collecting allfieldandfieldCtxobjects in a hugerootCtx.fieldsmap, and by not computing their depth (forshouldInstrument) from the path array even when unnecessary (like previously done in optimize graphql resolver depth calculation #1370, now with a workaround for [BUG?]: graphql resolver depth filter depends on path collapsing #7468).finishTimeof a collapsed-field span is now the time when the last resolver execution ended. I still don't completely understand how this worked previously (after Fix graphql.resolve span durations #4387 and remove async storage from graphql #5966), did it just use the finish time of the resolver that was started first?While the tests are still passing, I may have broken a few things that are not caught there; especially I'm uncertain about starting/finishing spans and how it works that they should enter/leave the
storage('legacy').getStore().Also I admit I didn't manage to get the benchmarks to run so I can't tell whether it's actually faster :-/
I've added lots of comments and fixed a few minor issues on the way. Also there's some points I'm still pondering:
GraphQLResolvePluginworks in completely different ways depending on whether thecollapseoption is disabled or not. I wonder if it may lead to cleaner code to split it into two plugins (with samestatic idandoperation) of which at most one isenabledat any point in time; using helper functions for the few bits of shared codegrapqhl.executespan, not under the respectivegraphql.resolvespan; especially when these are not collapsed. I didn't really manage to change this, and if I did it would probably be a major-semver breaking change; but is there a particular reason behind this design?I'm looking forward to your input and any other thoughts!