use updated gateway build, implement fix and update tests

apollographql · Nov 9, 2022 · dff19ff · dff19ff
1 parent 515ae7f
commit dff19ff
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 27 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -39,7 +39,7 @@
   },
   "devDependencies": {
     "@apollo/client": "3.7.1",
-    "@apollo/gateway": "2.1.4",
+    "@apollo/gateway": "https://output.circle-artifacts.com/output/job/3ea84c49-ca22-4dba-be5d-31004cf8d66a/artifacts/0/storage/@apollo/gateway/gateway-2.1.4.tgz",
     "@apollo/server-plugin-landing-page-graphql-playground": "4.0.0",
     "@apollo/subgraph": "2.1.4",
     "@apollo/utils.createhash": "1.1.0",

diff --git a/packages/server/src/__tests__/plugin/usageReporting/statsErrors.test.ts b/packages/server/src/__tests__/plugin/usageReporting/statsErrors.test.ts
@@ -75,12 +75,12 @@ describe('TODO', () => {
             "publicCacheTtlCount": [],
             "registeredOperationCount": 0,
             "requestCount": 1,
-            "requestsWithErrorsCount": 0,
+            "requestsWithErrorsCount": 1,
             "requestsWithoutFieldInstrumentation": 1,
             "rootErrorStats": {
               "children": {},
-              "errorsCount": 0,
-              "requestsWithErrorsCount": 0,
+              "errorsCount": 2,
+              "requestsWithErrorsCount": 1,
             },
           }
         `);
@@ -173,8 +173,8 @@ describe('TODO', () => {
                   "requestsWithErrorsCount": 0,
                 },
               },
-              "errorsCount": 0,
-              "requestsWithErrorsCount": 0,
+              "errorsCount": 1,
+              "requestsWithErrorsCount": 1,
             },
           }
         `);

diff --git a/packages/server/src/plugin/inlineTrace/index.ts b/packages/server/src/plugin/inlineTrace/index.ts
@@ -105,6 +105,7 @@ export function ApolloServerPluginInlineTrace(
         },
 
         async didEncounterErrors({ errors }) {
+          // FIXME: do I need to filter out errors that have a `serviceName` here as well?
           treeBuilder.didEncounterErrors(errors);
         },
 

diff --git a/packages/server/src/plugin/traceTreeBuilder.ts b/packages/server/src/plugin/traceTreeBuilder.ts
@@ -105,16 +105,6 @@ export class TraceTreeBuilder {
 
   public didEncounterErrors(errors: readonly GraphQLError[]) {
     errors.forEach((err) => {
-      // This is an error from a federated service. We will already be reporting
-      // it in the nested Trace in the query plan.
-      //
-      // XXX This probably shouldn't skip query or validation errors, which are
-      //      not in nested Traces because format() isn't called in this case! Or
-      //      maybe format() should be called in that case?
-      if (err.extensions?.serviceName) {
-        return;
-      }
-
       // In terms of reporting, errors can be re-written by the user by
       // utilizing the `transformError` parameter.  This allows changing
       // the message or stack to remove potentially sensitive information.

diff --git a/packages/server/src/plugin/usageReporting/plugin.ts b/packages/server/src/plugin/usageReporting/plugin.ts
@@ -551,6 +551,7 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
           },
 
           async didEncounterSubsequentErrors(_requestContext, errors) {
+            // FIXME: do I need to filter out errors that have a `serviceName` here as well?
             treeBuilder.didEncounterErrors(errors);
           },
 
@@ -564,8 +565,19 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
             // Search above for a comment about "didResolveSource" to see which
             // of the pre-source-resolution errors we are intentionally avoiding.
             if (!didResolveSource) return;
-            if (requestContext.errors) {
-              treeBuilder.didEncounterErrors(requestContext.errors);
+
+            if (requestContext.metrics.nonFtv1Errors) {
+              treeBuilder.didEncounterErrors(requestContext.metrics.nonFtv1Errors);
+            } else if (requestContext.errors) {
+              treeBuilder.didEncounterErrors(requestContext.errors.filter(error => {
+                // This is an error from a federated service. We will already be reporting
+                // it in the nested Trace in the query plan.
+                //
+                // XXX This probably shouldn't skip query or validation errors, which are
+                //      not in nested Traces because format() isn't called in this case! Or
+                //      maybe format() should be called in that case?
+                return !error.extensions?.serviceName;
+              }));
             }
 
             // If there isn't any defer/stream coming later, we're done.
@@ -708,7 +720,8 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
               asTrace:
                 sendTraces &&
                 (!isExecutable || !!metrics.captureTraces) &&
-                sendOperationAsTrace(trace, statsReportKey),
+                sendOperationAsTrace(trace, statsReportKey) &&
+                !metrics.nonFtv1Errors?.length,
               referencedFieldsByType,
             });