Correctly count metrics w.r.t non-ftv1 errors

apollographql · Dec 6, 2022 · 4a5ae68 · 4a5ae68
1 parent 78863f6
commit 4a5ae68
Show file tree

Hide file tree

Showing 8 changed files with 114 additions and 71 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": "https://output.circle-artifacts.com/output/job/3ea84c49-ca22-4dba-be5d-31004cf8d66a/artifacts/0/storage/@apollo/gateway/gateway-2.1.4.tgz",
+    "@apollo/gateway": "https://pkg.csb.dev/apollographql/federation/commit/ca4c450d/@apollo/gateway",
     "@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/stats.test.ts b/packages/server/src/__tests__/plugin/usageReporting/stats.test.ts
@@ -501,6 +501,7 @@ describe('Add trace to report', () => {
       trace: baseTrace,
       asTrace: false,
       referencedFieldsByType,
+      nonFtv1Errors: [],
     });
 
     expect(report.tracesPerQuery['key']?.trace?.length).toBe(0);
@@ -517,6 +518,7 @@ describe('Add trace to report', () => {
       asTrace: true,
       referencedFieldsByType,
       maxTraceBytes: 10,
+      nonFtv1Errors: [],
     });
 
     expect(report.tracesPerQuery['key']?.trace?.length).toBe(0);
@@ -533,6 +535,7 @@ describe('Add trace to report', () => {
       asTrace: true,
       referencedFieldsByType,
       maxTraceBytes: 500 * 1024,
+      nonFtv1Errors: [],
     });
 
     expect(report.tracesPerQuery['key']?.trace?.length).toBe(1);
@@ -548,6 +551,7 @@ describe('Add trace to report', () => {
       trace: baseTrace,
       asTrace: true,
       referencedFieldsByType,
+      nonFtv1Errors: [],
     });
 
     expect(report.tracesPerQuery['key']?.trace?.length).toBe(1);

diff --git a/packages/server/src/__tests__/plugin/usageReporting/statsErrors.test.ts b/packages/server/src/__tests__/plugin/usageReporting/statsErrors.test.ts
@@ -79,8 +79,8 @@ describe('TODO', () => {
             "requestsWithoutFieldInstrumentation": 1,
             "rootErrorStats": {
               "children": {},
-              "errorsCount": 2,
-              "requestsWithErrorsCount": 1,
+              "errorsCount": 0,
+              "requestsWithErrorsCount": 0,
             },
           }
         `);
@@ -90,7 +90,6 @@ describe('TODO', () => {
       })
       .reply(200);
 
-    // const res =
     await gatewayServer.executeHTTPGraphQLRequest({
       httpGraphQLRequest: {
         method: 'POST',
@@ -101,8 +100,6 @@ describe('TODO', () => {
       context: async () => ({}),
     });
 
-    // expect(res).toMatchInlineSnapshot();
-
     await gatewayServer.stop();
     await errorFtv1Subgraph.stop();
     await errorNoFtv1Subgraph.stop();
@@ -173,8 +170,8 @@ describe('TODO', () => {
                   "requestsWithErrorsCount": 0,
                 },
               },
-              "errorsCount": 1,
-              "requestsWithErrorsCount": 1,
+              "errorsCount": 0,
+              "requestsWithErrorsCount": 0,
             },
           }
         `);
@@ -211,7 +208,6 @@ describe('TODO', () => {
       })
       .reply(200);
 
-    // const res =
     await gatewayServer.executeHTTPGraphQLRequest({
       httpGraphQLRequest: {
         method: 'POST',
@@ -222,8 +218,6 @@ describe('TODO', () => {
       context: async () => ({}),
     });
 
-    // expect(res).toMatchInlineSnapshot();
-
     await gatewayServer.stop();
     await errorFtv1Subgraph.stop();
     await errorNoFtv1Subgraph.stop();

diff --git a/packages/server/src/plugin/inlineTrace/index.ts b/packages/server/src/plugin/inlineTrace/index.ts
@@ -105,7 +105,6 @@ 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,6 +105,16 @@ 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,7 +551,6 @@ 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);
           },
 
@@ -566,18 +565,8 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
             // of the pre-source-resolution errors we are intentionally avoiding.
             if (!didResolveSource) return;
 
-            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 (requestContext.errors) {
+              treeBuilder.didEncounterErrors(requestContext.errors);
             }
 
             // If there isn't any defer/stream coming later, we're done.
@@ -723,6 +712,7 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
                 sendOperationAsTrace(trace, statsReportKey) &&
                 !metrics.nonFtv1Errors?.length,
               referencedFieldsByType,
+              nonFtv1Errors: metrics.nonFtv1Errors ?? [],
             });
 
             // If the buffer gets big (according to our estimate), send.