feat(reporting): Add `reportTiming` option to `EngineReportingOptions` #3918

jsegaran · 2020-03-24T17:21:37Z

This PR adds a reportTiming api. It takes a function with parameter GraphQLRequestContextDidResolveOperation or GraphQLRequestContextDidEncounterErrors which returns a promise resolving to a boolean, or it takes a boolean. If the api resolves to false the agent will not instrument the operation or report the query. If the return type is true the agent will act as default and instrument and report the query to Apollo Graph Manager.

Create a new reportTiming api under EngineReportingOption that allows configuration of whether an operation should be traced. Previously there was no way to turn off instrumentation and tracing of requests if an apiKey was provided. The api allows turning off tracing on all requests or selectively turning off tracing based on a specific request shape.

The option can be set via the engine config, and the default is true where every trace gets instrumented and sent to Apollo Graph Manager.

const server = new ApolloServer({
  typeDefs,
  resolvers,
  engine: {
   reportTiming: true,
    /* Other existing options can remain the same. */
  },
});

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

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

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

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

sankhasp · 2020-04-24T15:30:09Z

@jsegaran great to see this change is being made, we at Glassdoor are eagerly waiting for this change to get published. @trevor-scheer @abernix -- Thanks for your reviews.

abernix · 2020-04-24T15:36:16Z

@sankhasp Just to set expectations — this is currently dependent on #3998 to avoid having to immediately revisit this once we land that (which makes more substantial changes). As soon as that's landed, this is up for rebasing and review and I expect this PR will make progress next week.

abernix

Looking good! I've left a couple comments/thoughts within.

Additionally, this should:

Be re-based on the release-2.14.0 branch now that Add support for willResolveField and corresponding end handler. #3988 has landed there and is the new base for the apollo-engine-reporting plugin.
Introduce some tests within the apollo-engine-reporting package that ensures this is behaving as expected.
~~Introduce some~~ (Copy paste fail)

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

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

I need to re-review.

abernix

Very excited for this to land! I'm thinking we'll get this into the 2.14.0 release, and I've left a few comments within that seem worth addressing.

Thanks for working on this! 🎉

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

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

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

packages/apollo-engine-reporting/src/__tests__/plugin.test.ts

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

jsegaran · 2020-05-18T02:17:50Z

Thanks for the review. I rebased the PR off #4084 since I assume that will be merged first.

abernix · 2020-06-12T12:59:09Z

To match reportSchema from #4236, I'm voting for reportTrace.

abernix · 2020-06-12T13:07:25Z

Just noticed the commit that was pushed to this that switched to timeOperation.

Whatever decision is made, please update the title and description of the PR. 😄

glasser · 2020-06-12T14:34:24Z

I agree with Jesse that the PR desc needs to be updated and that it would make more sense for the name to be parallel with reportSchema but this is fine.

) Co-authored-by: Renovate Bot <bot@renovateapp.com>

Co-authored-by: Renovate Bot <bot@renovateapp.com>

jsegaran · 2020-06-15T16:14:40Z

Updated the name to reportTiming and updated the pr description

glasser

I didn't do a full rereview but assuming you consistently got the name changed I think this is still fine?

docs/source/api/apollo-server.md

abernix

LGTM, though similarly to @glasser, I have not given it a full re-review though I've looked at it in the past. I'm going to retarget this to merge to release-2.15.0. Given the commits in this branch, is this a candidate for squashing?

jsegaran · 2020-06-16T13:58:45Z

Yea I think squashing this seems good.

abernix · 2020-06-17T18:24:48Z

Shipped in 2.15.0!

Previously, we were leveraging `sendReportsImmediately` to ... do exactly that even though these tests don't need to send reports _at all_!. This was purely a hack to make sure that we don't need to wait for the Engine Reporting Agent to eventually fail (after 5 seconds or whatever its reporting interval is). This new `reportTiming` option is available with the below-linked PR, so we can now clean this up a bit. This has the added advantage of not hitting our actual reporting endpoint with an invalid API key, and having the logs spew errors about failures in delivering those doomed reports. Ref: #3918

jsegaran force-pushed the jsegaran/should_report_api branch 2 times, most recently from 39d97cb to b3f6862 Compare March 24, 2020 19:00

jsegaran requested a review from abernix March 25, 2020 16:30

trevor-scheer requested changes Apr 3, 2020

View reviewed changes

abernix added the 🧮 usage-reporting label Apr 24, 2020

abernix force-pushed the jsegaran/should_report_api branch from a751239 to 41f2129 Compare May 4, 2020 09:14

jsegaran requested a review from trevor-scheer May 6, 2020 15:08

abernix self-assigned this May 11, 2020

abernix previously requested changes May 12, 2020

View reviewed changes

jsegaran force-pushed the jsegaran/should_report_api branch 2 times, most recently from f36fcab to 1b07df0 Compare May 13, 2020 05:29

jsegaran changed the base branch from master to release-2.14.0 May 13, 2020 05:29

jsegaran force-pushed the jsegaran/should_report_api branch from 1b07df0 to 334d77e Compare May 15, 2020 00:44

jsegaran requested a review from abernix May 15, 2020 01:11

jsegaran force-pushed the jsegaran/should_report_api branch 2 times, most recently from e552ba9 to ce57d65 Compare May 15, 2020 01:38

jsegaran requested a review from glasser May 15, 2020 01:40

abernix requested changes May 15, 2020

View reviewed changes

abernix added this to the Release 2.14.0 milestone May 15, 2020

jsegaran force-pushed the jsegaran/should_report_api branch 3 times, most recently from 4b64147 to 66559c8 Compare May 18, 2020 02:15

jsegaran changed the base branch from release-2.14.0 to jsegaran/schema_reporting May 18, 2020 02:15

jsegaran requested a review from abernix May 18, 2020 02:16

jsegaran force-pushed the jsegaran/schema_reporting branch from f7d857d to 977a89e Compare May 18, 2020 02:30

jsegaran force-pushed the jsegaran/should_report_api branch from 66559c8 to fdf2445 Compare May 18, 2020 02:30

abernix mentioned this pull request Jun 12, 2020

schema-reporting: Add startup logging to schema reporting #4240

Merged

jsegaran changed the title ~~feat(reporting): Add traceReporting option to EngineReportingOptions~~ feat(reporting): Add reportTiming option to EngineReportingOptions Jun 12, 2020

renovate bot and others added 8 commits June 12, 2020 22:21

chore(deps): update dependency gatsby-theme-apollo-docs to v4.2.6 (#4243

87b75c7

) Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency @types/aws-lambda to v8.10.56 (#4247)

f7a2684

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency @types/ioredis to v4.16.5 (#4248)

17acebd

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency @types/uuid to v7.0.4 (#4250)

1cc7ec0

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency gatsby to v2.23.3 (#4251)

1ce44e9

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency @types/supertest to v2.0.9 (#4249)

1434080

Co-authored-by: Renovate Bot <bot@renovateapp.com>

chore(deps): update dependency lerna to v3.22.1 (#4252)

bbed182

Co-authored-by: Renovate Bot <bot@renovateapp.com>

Rename to report timing

5ea850f

jsegaran and others added 2 commits June 15, 2020 09:15

Merge branch 'master' into jsegaran/should_report_api

029c153

Update changelog

7f464ab

glasser approved these changes Jun 15, 2020

View reviewed changes

abernix reviewed Jun 16, 2020

View reviewed changes

docs/source/api/apollo-server.md Outdated Show resolved Hide resolved

Use code fences in API documentation

5eb1778

abernix approved these changes Jun 16, 2020

View reviewed changes

abernix changed the base branch from master to release-2.15.0 June 16, 2020 12:24

jsegaran merged commit 54ff1e6 into release-2.15.0 Jun 16, 2020

abernix mentioned this pull request Jun 29, 2020

chore(op-reg): adjustments and cleanup to existing tests. #4324

Merged

abernix deleted the jsegaran/should_report_api branch February 5, 2021 06:58

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

feat(reporting): Add `reportTiming` option to `EngineReportingOptions` #3918

feat(reporting): Add `reportTiming` option to `EngineReportingOptions` #3918

jsegaran commented Mar 24, 2020 •

edited

Loading

sankhasp commented Apr 24, 2020

abernix commented Apr 24, 2020

abernix left a comment •

edited

Loading

abernix left a comment

jsegaran commented May 18, 2020

abernix commented Jun 12, 2020

abernix commented Jun 12, 2020

glasser commented Jun 12, 2020

jsegaran commented Jun 15, 2020

glasser left a comment

abernix left a comment

jsegaran commented Jun 16, 2020

abernix commented Jun 17, 2020

feat(reporting): Add reportTiming option to EngineReportingOptions #3918

feat(reporting): Add reportTiming option to EngineReportingOptions #3918

Conversation

jsegaran commented Mar 24, 2020 • edited Loading

sankhasp commented Apr 24, 2020

abernix commented Apr 24, 2020

abernix left a comment • edited Loading

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

jsegaran commented May 18, 2020

abernix commented Jun 12, 2020

abernix commented Jun 12, 2020

glasser commented Jun 12, 2020

jsegaran commented Jun 15, 2020

glasser left a comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

jsegaran commented Jun 16, 2020

abernix commented Jun 17, 2020

feat(reporting): Add `reportTiming` option to `EngineReportingOptions` #3918

feat(reporting): Add `reportTiming` option to `EngineReportingOptions` #3918

jsegaran commented Mar 24, 2020 •

edited

Loading

abernix left a comment •

edited

Loading