Fix error reporting when trace data is not included #7136

trevor-scheer · 2022-11-09T22:35:34Z

This is a 2-part change which requires an update to the gateway runtime as well. Related PR: apollographql/federation#2242

It was brought to our attention that trace information w.r.t. subgraph errors is incorrect in the case that there is no trace in the subgraph response (i.e. no ftv1 support in the subgraph or fieldLevelInstrumentation is 0 or returns 0).

With this change:

Errors reported by subgraphs (with no trace data in the response) are now accurately reflected in the numeric error stats
Operations that receive errors from subgraphs (with no trace data in the response) are no longer sent as incomplete, error-less traces

The gateway change now collects errors separately when no trace data is found and passes that error information (subgraph and error path) along with the other metrics data for the plugin to consume. The plugin uses the subgraph name and error path in order to add error stat information to the tree.

netlify · 2022-11-09T22:35:39Z

✅ Deploy Preview for apollo-server-docs ready!

Name	Link
🔨 Latest commit	`5548ec0`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/63e168ff4abc640009deab40
😎 Deploy Preview	https://deploy-preview-7136--apollo-server-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codesandbox-ci · 2022-11-09T22:36:24Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5548ec0:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

update CS:CI config to build missing packages

glasser

The broad structure here seems good but the tests suggest there's a bug preventing the fix from working. (Maybe the version of gateway loaded is wrong?)

Also I'm sure you know this but a ton more comments would be helpful, both about the new stuff and about the other stuff we've discovered today that wasn't obvious.

glasser · 2022-12-06T20:53:48Z

package.json

@@ -39,7 +39,9 @@
  },
  "devDependencies": {
    "@apollo/client": "3.7.1",
+    "@apollo/gateway": "https://pkg.csb.dev/apollographql/federation/commit/ca4c450d/@apollo/gateway",


reminder to fix this. (is there a circular dependency issue where we can't publish one until the other is done and vice versa? this is just for tests though right?)

The "correct" way to do this would be publish gateway interface -> gateway -> server, but yes it is just tests.

packages/gateway-interface/src/index.ts

packages/server/src/__tests__/plugin/usageReporting/statsErrors.test.ts

glasser

Other than those minor notes this looks good. The PR title/description could use an update to describe this kinda subtle issue, and also a changeset.

packages/gateway-interface/src/index.ts

packages/server/src/plugin/usageReporting/plugin.ts

packages/server/src/plugin/usageReporting/stats.ts

Precursor to: #7136 apollographql/federation#2242 This interface needs to be published in order to properly land and publish the required change to gateway, which the Apollo Server change depends on.

@trevor-scheer

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-gateway-interface@1.1.0 ### Minor Changes - [#7325](#7325) [`e0f959a63`](e0f959a) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add optional `nonFtv1ErrorPaths` to Gateway metrics data. This change is a prerequisite to: - <apollographql/federation#2242> - <#7136> ## @apollo/server-integration-testsuite@4.3.2 ### Patch Changes - [#7316](#7316) [`37d884650`](37d8846) Thanks [@renovate](https://github.com/apps/renovate)! - Update graphql-http dependency - Updated dependencies \[[`f246ddb71`](f246ddb), [`e25cb58ff`](e25cb58)]: - @apollo/server@4.3.2 ## @apollo/server@4.3.2 ### Patch Changes - [#7314](#7314) [`f246ddb71`](f246ddb) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add an `__identity` property to `HeaderMap` class to disallow standard `Map`s (in TypeScript). This ensures that typechecking occurs on fields which are declared to accept a `HeaderMap` (notably, the `httpGraphQLRequest.headers` option to `ApolloServer.executeHTTPGraphQLRequest` and the `http.headers` option to `ApolloServer.executeOperation`). This might be a breaking change for integration authors, but should be easily fixed by switching from `new Map<string, string>()` to `new HeaderMap()`. - [#7326](#7326) [`e25cb58ff`](e25cb58) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Pin `node-abort-controller` version to avoid breaking change. Apollo Server users can enter a broken state if they update their package-lock.json due to a breaking change in a minor release of the mentioned package. Ref: <southpolesteve/node-abort-controller#39> - Updated dependencies \[[`e0f959a63`](e0f959a)]: - @apollo/server-gateway-interface@1.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

This commit precedes apollographql/apollo-server#7136 For the mentioned PR (full context within), the gateway needs to capture error information from subgraphs with no trace data separately and report them within the metrics object that's shared with the inline trace plugin. This change only takes effect for subgraphs with no ftv1 support or when fieldLevelInstrumentation on the inline trace plugin is set to 0 or returns 0.

glasser · 2023-01-26T19:01:22Z

.changeset/heavy-poets-wink.md

+'@apollo/server': patch
+---
+
+Errors reported by subgraphs (with no trace data in the response) are now accurately reflected in the numeric error stats.


Does this changeset (and PR description) need to note that a particular version of Gateway is also needed to have the issue be actually fixed?

Added in e1b8122 (with a FIXME to plug in final gateway version)

glasser · 2023-01-26T19:01:33Z

package.json

@@ -38,7 +38,9 @@
  },
  "devDependencies": {
    "@apollo/client": "3.7.5",
+    "@apollo/gateway": "https://pkg.csb.dev/apollographql/federation/commit/166fdd24/@apollo/gateway",


Would be nice to avoid this but I guess the gateway release process is slow.

This will become the actual gateway version once the patch is released, won't land this until then

glasser · 2023-01-26T19:01:40Z

package.json

    "@apollo/server-plugin-landing-page-graphql-playground": "4.0.0",
+    "@apollo/subgraph": "https://pkg.csb.dev/apollographql/federation/commit/166fdd24/@apollo/subgraph",


Why is this needed?

Probably could've always been just the current version of subgraph but this will be latest once the patch release is published

@trevor-scheer

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-integration-testsuite@4.3.3 ### Patch Changes - [#7338](#7338) [`01bc39838`](01bc398) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update graphql-http to 1.13.0 - Updated dependencies \[[`9de18b34c`](9de18b3), [`8c635d104`](8c635d1)]: - @apollo/server@4.3.3 ## @apollo/server@4.3.3 ### Patch Changes - [#7331](#7331) [`9de18b34c`](9de18b3) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Unpin `node-abort-controller` and update to latest unbreaking patch - [#7136](#7136) [`8c635d104`](8c635d1) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Errors reported by subgraphs (with no trace data in the response) are now accurately reflected in the numeric error stats. Operations that receive errors from subgraphs (with no trace data in the response) are no longer sent as incomplete, error-less traces. Note: in order for this fix to take effect, your `@apollo/gateway` version must be updated to v2.3.1 or later. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

trevor-scheer force-pushed the trevor/usage-reporting-errors branch 4 times, most recently from dff19ff to 78863f6 Compare November 10, 2022 00:00

trevor-scheer added 4 commits December 6, 2022 12:24

write test demonstrating current behavior

4bee898

update metrics interface

9509307

update CS:CI config to build missing packages

use updated gateway build, implement fix and update tests

760c56a

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

9efb148

trevor-scheer force-pushed the trevor/usage-reporting-errors branch from 4a5ae68 to 9efb148 Compare December 6, 2022 20:30

trevor-scheer added 2 commits December 6, 2022 12:43

package-lock update

6adca8b

polish

0621c25

glasser requested changes Dec 6, 2022

View reviewed changes

trevor-scheer added 6 commits January 18, 2023 16:45

Merge branch 'main' into trevor/usage-reporting-errors

674b565

Update nonFtv1Errors -> nonFtv1ErrorPaths

7d4804b

prettier

82afd7c

Incorporate subgraph name into root key

4a6f39e

snapshot updates

0367f6a

extract and export NonFtv1ErrorPath interface

52f8b64

glasser reviewed Jan 24, 2023

View reviewed changes

packages/gateway-interface/src/index.ts Outdated Show resolved Hide resolved

packages/server/src/plugin/usageReporting/plugin.ts Outdated Show resolved Hide resolved

packages/server/src/plugin/usageReporting/stats.ts Outdated Show resolved Hide resolved

glasser mentioned this pull request Jan 24, 2023

Capture non-ftv1 errors in metrics apollographql/federation#2242

Merged

trevor-scheer added 4 commits January 24, 2023 15:39

remove outdated comment

12355d9

add comments

5c19b5a

remove unused import

57dd284

Complicate test cases for deeper errors including _entities

c3ba88e

trevor-scheer changed the title ~~WIP: Fix error reporting when trace data is not included~~ Fix error reporting when trace data is not included Jan 25, 2023

trevor-scheer mentioned this pull request Jan 25, 2023

Update gateway metrics interface #7325

Merged

github-actions bot mentioned this pull request Jan 25, 2023

Version Packages #7315

Merged

github-actions bot mentioned this pull request Jan 25, 2023

Version Packages SimenB/apollo-server#298

Closed

Merge branch 'main' into trevor/usage-reporting-errors

b2b98f6

changeset

e167da0

This was referenced Jan 26, 2023

release: on branch main apollographql/federation#2357

Merged

release: on branch main NOUIY/federation#196

Open

glasser reviewed Jan 26, 2023

View reviewed changes

add note about updating gateway

e1b8122

trevor-scheer added 2 commits February 6, 2023 12:52

Update to latest gateway version with necessary patch release

d6c5293

Merge branch 'main' into trevor/usage-reporting-errors

5548ec0

trevor-scheer merged commit 8c635d1 into main Feb 6, 2023

trevor-scheer deleted the trevor/usage-reporting-errors branch February 6, 2023 21:59

github-actions bot mentioned this pull request Feb 6, 2023

Version Packages #7333

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error reporting when trace data is not included #7136

Fix error reporting when trace data is not included #7136

trevor-scheer commented Nov 9, 2022 •

edited

Loading

netlify bot commented Nov 9, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 9, 2022 •

edited

Loading

glasser left a comment

glasser Dec 6, 2022

trevor-scheer Dec 6, 2022

glasser left a comment

glasser Jan 26, 2023

trevor-scheer Jan 26, 2023

glasser Jan 26, 2023

trevor-scheer Jan 26, 2023

glasser Jan 26, 2023

trevor-scheer Jan 26, 2023

		"@apollo/server-plugin-landing-page-graphql-playground": "4.0.0",
		"@apollo/subgraph": "https://pkg.csb.dev/apollographql/federation/commit/166fdd24/@apollo/subgraph",

Fix error reporting when trace data is not included #7136

Fix error reporting when trace data is not included #7136

Conversation

trevor-scheer commented Nov 9, 2022 • edited Loading

netlify bot commented Nov 9, 2022 • edited Loading

✅ Deploy Preview for apollo-server-docs ready!

codesandbox-ci bot commented Nov 9, 2022 • edited Loading

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a 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

trevor-scheer commented Nov 9, 2022 •

edited

Loading

netlify bot commented Nov 9, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 9, 2022 •

edited

Loading