apollo-engine-reporting: update to newer plugin interface #2441

glasser · 2019-03-14T01:16:28Z

This update mostly migrates from the old GraphQLExtension API to the newer
ApolloServerPlugin API.

This extends the plugin interface with a few new methods to support
apollo-engine-reporting. It does not yet add willResolveField, so for the short
term it adds a hacky way for plugins to expose GraphQLExtensions just for the
sake of willResolveField.

The interface for actually using Engine reporting remains the same: passing
configuration to the engine parameter of the ApolloServer constructor.

This includes:

Changing GraphQLResponse to have the GraphQLFormattedError rather than
GraphQLError (which among other things will be easier to serialize for a
response cache). Add an explicit didEncounterErrors hook that gets the
GraphQLError first. Now we don't have to worry if a plugin/extension gets
willSendResponse called before or after FormatErrorExtension gets to it!
FormatErrorExtension is removed. This part was based on an implementation by
@martijnwalraven.
Add originalDocumentString, persistedQueryHit, and persistedQueryRegister to
GraphQLRequestContext.
Fix Dispatcher so it doesn't lose this when invoking methods on the
GraphQLRequestListener.
Add didResolveDocument to the plugin class, which is run after resolving
APQs.

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

glasser · 2019-03-14T01:53:56Z

Hmm, it builds for me locally. Advice on what build files need to be tweaked would be helpful.

martijnwalraven · 2019-03-14T11:29:32Z

packages/apollo-server-core/src/requestPipelineAPI.ts


  // `operationName` is set based on the operation AST, so it is defined
  // even if no `request.operationName` was passed in.
  // It will be set to `null` for an anonymous operation.
  readonly operationName?: string | null;
  readonly operation?: OperationDefinitionNode;

+  readonly persistedQueryHit?: boolean;
+  readonly persistedQueryRegister?: boolean;


I was thinking we may want to put persistedQueryHit and persistedQueryRegister on a metrics property (of type GraphQLRequestMetrics), to group these together with information about cache hits.

martijnwalraven · 2019-03-14T11:29:49Z

packages/apollo-server-core/src/utils/dispatcher.ts

@@ -21,7 +21,7 @@ export class Dispatcher<T> {
      this.targets.map(target => {
        const method = target[methodName];
        if (method && typeof method === 'function') {
-          return method(...args);
+          return method.apply(target, args);


Good catch! Sorry I missed this.

martijnwalraven · 2019-03-14T11:33:22Z

packages/apollo-server-core/src/requestPipelineAPI.ts

@@ -50,13 +52,17 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {
  readonly queryHash?: string;

  readonly document?: DocumentNode;
+  readonly originalDocumentString?: string;


I'm still not really sure this is what we want, because the cache key shouldn't depend on the whole document if it contains more than one operation. So ideally we'd have a well-defined uniform notion of an operationID. But even if we do want to depend on the whole document for now, couldn't we use the queryHash instead? (We should probably rename that to documentHash.)

This also only gets used for parse failures. But it gets sent to Engine and used to display the (failing) query in the UI. So it needs to be text, not a hash.

But I'm going to call it originalDocumentText for now.

Re caching, which is a separate PR for now but the idea here, I think we're being strict enough about "this is a full request cache" and ignoring other semantically equivalent changes that "you have to be consistent about what the whole document looks like" seems like a fair place to start for v1 at least.

Or maybe just documentText, since I think that captures the "original" concept to some degree.

Yep, I like documentText!

martijnwalraven · 2019-03-14T11:43:28Z

packages/apollo-server-plugin-base/src/index.ts

@@ -20,6 +21,15 @@ export interface ApolloServerPlugin {
 }

 export interface GraphQLRequestListener<TContext = Record<string, any>> {
+  didResolveDocument?(


I'm not sure I see why this hook is needed, because it seems we could just as well rely on didResolveOperation? If we do need it, I think the name didResolveDocument may be a bit confusing if we don't have document (the parsed document) yet at this point. Especially because we have a document cache, it seems fine to invoke it after we're guaranteed to have a document though. (I may be missing something, but I couldn't find where this is invoked from.)

This gets the text in even if parsing fails.

Ah, that makes sense. I like the name didResolveDocument, but since we already use document in the sense of parsed document that may be confusing. Not sure what the right solution is here. We could conceivably rename document to documentAST, although that would be breaking. It would also free up document to mean the unparsed document though (what is currently called originalDocumentString).

Maybe didResolveDocumentText or didResolveDocumentString? I kind of like text even though it's not a term used elsewhere.

Yep, didResolveDocumentText to be consistent with documentText.

glasser · 2019-03-14T23:16:49Z

OK, this takes most of the suggestions above into account (see separate fixup! commits) and also pulls in an executor implementation.

glasser · 2019-03-15T22:22:17Z

I think this PR is in a state where it could hypothetically be merged. (Do a rebase -i to squash the fixup commits together.)

@martijnwalraven If I'm not on leave Monday, hopefully you will be able to give it another round of review on Monday?

martijnwalraven · 2019-03-19T11:24:33Z

packages/apollo-engine-reporting/src/extension.ts

+      this.trace.persistedQueryRegister = true;
+    }
+
+    // Generally, we'll get queryString here and not parsedQuery; we only get


This comment should probably be removed.

martijnwalraven · 2019-03-19T11:30:08Z

packages/apollo-server-core/src/utils/dispatcher.ts

@@ -9,7 +9,7 @@ type UnwrapPromise<T> = T extends Promise<infer U> ? U : T;
 type DidEndHook<TArgs extends any[]> = (...args: TArgs) => void;

 export class Dispatcher<T> {
-  constructor(protected targets: T[]) {}
+  constructor(public requestListeners: T[]) {}


My idea was that we'd be able to keep Dispatcher generic and possibly extract it into a utility package at some point. I realize it's already bound to the request listener API because it has special provisions for DidEndHook, but renaming targets to requestListeners makes it even more unlikely we'll be able to reuse this.

martijnwalraven · 2019-03-19T11:33:47Z

packages/apollo-engine-reporting/src/extension.ts

+  ) {
+    // XXX Is it safe to use willSendReponse to do this finalization, vs
+    // something that's more explicitly part of a try/final in requestPipeline
+    // (eg, a new requestDidEnd callback?)


Good point. I think it's safe right now, but adding an explicit requestDidEnd hook is probably a better solution.

martijnwalraven · 2019-03-19T11:34:42Z

packages/apollo-engine-reporting/src/extension.ts

+      // We didn't get an AST, possibly because of a parse failure. Let's just
+      // use the full query string.
+      //
+      // XXX This does mean that even if you use a calculateSignature which


This definitely seems important to resolve before we release this.

This isn't a change from the existing code (even the comment) but a user did notice it recently. Thoughts?

martijnwalraven · 2019-03-19T12:03:42Z

packages/apollo-server-core/src/requestPipeline.ts

+    });
+    executors.push(defaultExecutor);
+
+    for (const executor of executors) {


I was hoping we could extract this into something like Dispatcher.invokeHookSequentially, but I realize what's keeping that from working is the ability to pass in an executor outside of a plugin (which we want for the gateway).

To be honest, using the idea of an executor to return a response from cache still feels out of place to me. We always invoke executionDidStart/executionDidEnd for example, which I don't think we should do for caches. And we may want to rethink other hooks as well, based on whether an execution result or a preformatted response was returned. Right now for example, executors are defined as returning an ExecutionResult, which contains an errors property with GraphQLError instances. Those will need to be fed through formatError to be serializable. A response from cache on the other hand, will contain GraphQLFormattedErrors.

An alternative might be to separate the notion of an executor from the ability to short-circuit execution through something like responseForOperation (which we would use for caching). We could then define executor as config.executor || defaultExecutor, similar to how the fieldResolver property is used in graphql-js.

Ok, you've said this like 5 times now so perhaps I will listen. In that case I'll just back out the executor change entirely and let it be its own PR.

Or really, that means just dropping this PR and rebasing the cache PR on master.

@martijnwalraven

This update mostly migrates from the old GraphQLExtension API to the newer ApolloServerPlugin API. This extends the plugin interface with a few new methods to support apollo-engine-reporting. It does not yet add willResolveField, so for the short term it adds a hacky way for plugins to expose GraphQLExtensions just for the sake of willResolveField. The interface for actually using Engine reporting remains the same: passing configuration to the `engine` parameter of the ApolloServer constructor. This includes: - Changing GraphQLResponse to have the GraphQLFormattedError rather than GraphQLError (which among other things will be easier to serialize for a response cache). Add an explicit didEncounterErrors hook that gets the GraphQLError first. Now we don't have to worry if a plugin/extension gets willSendResponse called before or after FormatErrorExtension gets to it! FormatErrorExtension is removed. This part was based on an implementation by @martijnwalraven. - Add originalDocumentString, persistedQueryHit, and persistedQueryRegister to GraphQLRequestContext. - Fix Dispatcher so it doesn't lose `this` when invoking methods on the GraphQLRequestListener. - Add didResolveDocument to the plugin class, which is run after resolving APQs.

Add `apollo-server-plugin-base` dependency to `apollo-engine-reporting` TypeScript project

Code review feedback

Re-separate Dispatcher and GraphQLExtensionStack initialization. Expose requestListeners on Dispatcher instead. Will use the same field to get executors. Also add a missing TContext on GraphQLRequestListener

@martijnwalraven

Original implementation by @martijnwalraven. This implementation should support both the response cache and the gateway.

glasser · 2019-03-28T17:27:08Z

No longer needed for the cache plugin.

glasser requested a review from martijnwalraven March 14, 2019 01:16

glasser requested a review from abernix as a code owner March 14, 2019 01:16

martijnwalraven reviewed Mar 14, 2019

View reviewed changes

glasser force-pushed the glasser/reporting-as-plugin branch from 7e3c694 to 04283a4 Compare March 14, 2019 23:16

glasser force-pushed the glasser/reporting-as-plugin branch 2 times, most recently from 1f54df8 to d9570c7 Compare March 15, 2019 16:56

glasser mentioned this pull request Mar 15, 2019

Full query response cache plugin #2437

Merged

martijnwalraven reviewed Mar 19, 2019

View reviewed changes

glasser and others added 5 commits March 19, 2019 15:41

fixup! apollo-engine-reporting: update to newer plugin interface

573c89c

Add `apollo-server-plugin-base` dependency to `apollo-engine-reporting` TypeScript project

fixup! apollo-engine-reporting: update to newer plugin interface

1c8f625

Code review feedback

fixup! apollo-engine-reporting: update to newer plugin interface

b098be6

Re-separate Dispatcher and GraphQLExtensionStack initialization. Expose requestListeners on Dispatcher instead. Will use the same field to get executors. Also add a missing TContext on GraphQLRequestListener

Add pluggable executor to request pipeline

2e5deff

Original implementation by @martijnwalraven. This implementation should support both the response cache and the gateway.

glasser force-pushed the glasser/reporting-as-plugin branch from d9570c7 to 2e5deff Compare March 19, 2019 22:42

glasser closed this Mar 28, 2019

abernix mentioned this pull request Apr 10, 2019

[release-2.5.0] Fix missing operationName in apollo-engine-reporting. #2557

Merged

abernix deleted the glasser/reporting-as-plugin branch February 5, 2021 07:05

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apollo-engine-reporting: update to newer plugin interface #2441

apollo-engine-reporting: update to newer plugin interface #2441

glasser commented Mar 14, 2019

glasser commented Mar 14, 2019

martijnwalraven Mar 14, 2019

glasser Mar 14, 2019

martijnwalraven Mar 14, 2019

martijnwalraven Mar 14, 2019

glasser Mar 14, 2019

glasser Mar 14, 2019

glasser Mar 14, 2019

martijnwalraven Mar 15, 2019

martijnwalraven Mar 14, 2019

glasser Mar 14, 2019

martijnwalraven Mar 14, 2019

glasser Mar 14, 2019

martijnwalraven Mar 15, 2019

glasser commented Mar 14, 2019

glasser commented Mar 15, 2019

martijnwalraven Mar 19, 2019

martijnwalraven Mar 19, 2019

martijnwalraven Mar 19, 2019

martijnwalraven Mar 19, 2019

glasser Mar 19, 2019

martijnwalraven Mar 19, 2019

glasser Mar 19, 2019

glasser Mar 19, 2019

glasser commented Mar 28, 2019

apollo-engine-reporting: update to newer plugin interface #2441

apollo-engine-reporting: update to newer plugin interface #2441

Conversation

glasser commented Mar 14, 2019

glasser commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Mar 14, 2019

glasser commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Mar 28, 2019