Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Graph Manager (Engine) reporting "extensions" become "plugins". #3998

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
de7ba72
refactor: Graph Manager (Engine) reporting "extensions" become "plugi…
abernix Mar 30, 2020
78a4cb7
fix: Keep special-cased errors (e.g. APQ not found) as unreported.
abernix Apr 14, 2020
c67a6df
eliminate!: Remove now deprecated `EngineReportingExtension`.
abernix Apr 15, 2020
dce4a24
fix!: Rename AERs `newExtension` to `newPlugin` to match new usage.
abernix Apr 27, 2020
a8ab841
no-op: Add back comment about `ftv1` trace format.
abernix Apr 27, 2020
fc05e8d
Use optional chaining when accessing optional `request.http.headers`.
abernix Apr 28, 2020
48eab7e
Ensure `metrics` is present before plugin initialization.
abernix Apr 28, 2020
8ca5d11
Remove guard around `metrics` which is unnecessary after 48eab7efa.
abernix Apr 28, 2020
0658df5
Move `Trace.HTTP` init inside of `ensurePreflight`.
abernix Apr 28, 2020
fc966a4
Remove unnecessary `queryString` assignment and related comment.
abernix Apr 28, 2020
766c738
Destructure some `requestContext` properties for brevity.
abernix Apr 28, 2020
f4aad30
chore!: Use `document` / `source` rather than `documentAST` / `queryS…
abernix Apr 28, 2020
6b0c83c
Expand on comment about the `ftv1` extension.
abernix Apr 28, 2020
48b8c95
Merge branch 'abernix/add-willResolveField-and-didResolveField' into …
abernix Apr 29, 2020
283e82f
Tweak err. message when `ftv1` is already present in `extensions` res…
abernix May 7, 2020
e0949ec
Remove `TODO` comment I suggested I'd remove!
abernix May 7, 2020
0a0bd14
Merge branch 'abernix/add-willResolveField-and-didResolveField' into …
abernix May 7, 2020
1756b37
Merge branch 'abernix/add-schema-and-schemaHash-to-requestContext' in…
abernix May 7, 2020
e813e5b
refactor(tests): Better helpers for APQ tests in intgr. testsuite.
abernix May 7, 2020
d913589
wip didResolveSource
abernix May 7, 2020
39ff086
other stuff
abernix May 7, 2020
4be279d
Merge branch 'abernix/add-schema-and-schemaHash-to-requestContext' in…
abernix May 8, 2020
ba37e68
changelog: #3998
abernix May 8, 2020
e9edd9a
types(e-r): Improve the typings of `didEnd`.
abernix May 8, 2020
b981b59
chore(e-r): Eliminate `ensurePreflight` by using (new) `didResolveSou…
abernix May 8, 2020
6d4777d
changelog: Add note that I believe some new APQ errors are now traced.
abernix May 8, 2020
bacbb17
Revert "changelog: Add note that I believe some new APQ errors are no…
abernix May 8, 2020
59b4013
fix(e-r): Do not keep traces unless we resolve the "source".
abernix May 8, 2020
16e471e
Merge branch 'abernix/add-schema-and-schemaHash-to-requestContext' in…
abernix May 8, 2020
b0948a8
fix: Preserve client-requested `operationName` on op. name resolution…
abernix May 8, 2020
7c8b8e3
Merge branch 'abernix/add-schema-and-schemaHash-to-requestContext' in…
abernix May 8, 2020
62fc270
Switch to new `willResolveField` object parameter, rather position.
abernix May 8, 2020
c73cd16
nit: Add missing closing paren on comment
abernix May 11, 2020
af2d6d8
Merge branch 'abernix/add-willResolveField-and-didResolveField' into …
abernix May 11, 2020
1144c7a
Merge branch 'release-2.14.0' into abernix/migrate-engine-reporting-e…
abernix May 12, 2020
3ccccad
Merge remote-tracking branch 'origin/release-2.14.0' into abernix/mig…
abernix May 12, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,11 @@ The version headers in this history reflect the versions of Apollo Server itself

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- `apollo-engine-reporting`: The underlying integration of this plugin, which instruments and traces the graph's resolver performance and transmits these metrics to [Apollo Graph Manager](https://engine.apollographql.com/), has been changed from the (soon to be deprecated) `graphql-extensions` API to the new [request pipeline `plugins` API](https://www.apollographql.com/docs/apollo-server/integrations/plugins/). [PR #3998](https://github.com/apollographql/apollo-server/pull/3998)

_This change should be purely an implementation detail for a majority of users_. There are, however, some special considerations which are worth noting:

- The federated tracing plugin's `ftv1` response on `extensions` (which is present on the response from an implementing service to the gateway) is now placed on the `extensions` _after_ the `formatResponse` hook. Anyone leveraging the `extensions`.`ftv1` data from the `formatResponse` hook will find that it is no longer present at that phase.
- `apollo-tracing`: This package's internal integration with Apollo Server has been switched from using the soon-to-be-deprecated`graphql-extensions` API to using [the request pipeline plugin API](https://www.apollographql.com/docs/apollo-server/integrations/plugins/). Behavior should remain otherwise the same. [PR #3991](https://github.com/apollographql/apollo-server/pull/3991)

### v2.13.0
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/apollo-engine-reporting/package.json
Expand Up @@ -18,7 +18,7 @@
"apollo-server-errors": "file:../apollo-server-errors",
"apollo-server-types": "file:../apollo-server-types",
"async-retry": "^1.2.1",
"graphql-extensions": "file:../graphql-extensions"
"apollo-server-plugin-base": "file:../apollo-server-plugin-base"
},
"peerDependencies": {
"graphql": "^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0"
Expand Down
@@ -1,19 +1,11 @@
import { makeExecutableSchema, addMockFunctionsToSchema } from 'graphql-tools';
import {
GraphQLExtensionStack,
enableGraphQLExtensions,
} from 'graphql-extensions';
import { graphql, GraphQLError } from 'graphql';
import { Request } from 'node-fetch';
import {
EngineReportingExtension,
makeTraceDetails,
makeHTTPRequestHeaders,
} from '../extension';
import { makeTraceDetails, makeHTTPRequestHeaders, plugin } from '../plugin';
import { Headers } from 'apollo-server-env';
import { InMemoryLRUCache } from 'apollo-server-caching';
import { AddTraceArgs } from '../agent';
import { Trace } from 'apollo-engine-reporting-protobuf';
import pluginTestHarness from 'apollo-server-core/dist/utils/pluginTestHarness';

it('trace construction', async () => {
const typeDefs = `
Expand Down Expand Up @@ -53,41 +45,33 @@ it('trace construction', async () => {

const schema = makeExecutableSchema({ typeDefs });
addMockFunctionsToSchema({ schema });
enableGraphQLExtensions(schema);

const traces: Array<AddTraceArgs> = [];
async function addTrace(args: AddTraceArgs) {
traces.push(args);
}

const reportingExtension = new EngineReportingExtension(
{},
addTrace,
'schema-hash',
);
const stack = new GraphQLExtensionStack([reportingExtension]);
const requestDidEnd = stack.requestDidStart({
request: new Request('http://localhost:123/foo'),
queryString: query,
requestContext: {
request: {
query,
operationName: 'q',
extensions: {
clientName: 'testing suite',
},
const pluginInstance = plugin({ /* no options!*/ }, addTrace);

pluginTestHarness({
abernix marked this conversation as resolved.
Show resolved Hide resolved
pluginInstance,
schema,
graphqlRequest: {
query,
operationName: 'q',
extensions: {
clientName: 'testing suite',
},
context: {},
cache: new InMemoryLRUCache(),
http: new Request('http://localhost:123/foo'),
},
executor: async ({ request: { query: source }}) => {
return await graphql({
schema,
source,
});
},
context: {},
});
await graphql({
schema,
source: query,
contextValue: { _extensionStack: stack },
});
requestDidEnd();

// XXX actually write some tests
});

Expand Down
41 changes: 19 additions & 22 deletions packages/apollo-engine-reporting/src/agent.ts
Expand Up @@ -11,10 +11,11 @@ import {
import { fetch, RequestAgent, Response } from 'apollo-server-env';
import retry from 'async-retry';

import { EngineReportingExtension } from './extension';
import { plugin } from './plugin';
import { GraphQLRequestContext, Logger, SchemaHash } from 'apollo-server-types';
import { InMemoryLRUCache } from 'apollo-server-caching';
import { defaultEngineReportingSignature } from 'apollo-graphql';
import { ApolloServerPlugin } from "apollo-server-plugin-base";

let warnedOnDeprecatedApiKey = false;

Expand Down Expand Up @@ -251,8 +252,8 @@ export interface AddTraceArgs {
operationName: string;
queryHash: string;
schemaHash: SchemaHash;
queryString?: string;
documentAST?: DocumentNode;
source?: string;
document?: DocumentNode;
}

const serviceHeaderDefaults = {
Expand Down Expand Up @@ -328,20 +329,16 @@ export class EngineReportingAgent<TContext = any> {
handleLegacyOptions(this.options);
}

public newExtension(schemaHash: SchemaHash): EngineReportingExtension<TContext> {
return new EngineReportingExtension<TContext>(
this.options,
this.addTrace.bind(this),
schemaHash,
abernix marked this conversation as resolved.
Show resolved Hide resolved
);
public newPlugin(): ApolloServerPlugin<TContext> {
return plugin(this.options, this.addTrace.bind(this));
}

public async addTrace({
trace,
queryHash,
documentAST,
document,
operationName,
queryString,
source,
schemaHash,
}: AddTraceArgs): Promise<void> {
// Ignore traces that come in after stop().
Expand All @@ -368,8 +365,8 @@ export class EngineReportingAgent<TContext = any> {

const signature = await this.getTraceSignature({
queryHash,
documentAST,
queryString,
document,
source,
operationName,
});

Expand Down Expand Up @@ -534,17 +531,17 @@ export class EngineReportingAgent<TContext = any> {
private async getTraceSignature({
queryHash,
operationName,
documentAST,
queryString,
document,
source,
}: {
queryHash: string;
operationName: string;
documentAST?: DocumentNode;
queryString?: string;
document?: DocumentNode;
source?: string;
}): Promise<string> {
if (!documentAST && !queryString) {
if (!document && !source) {
// This shouldn't happen: one of those options must be passed to runQuery.
throw new Error('No queryString or documentAST?');
throw new Error('No document or source?');
}

const cacheKey = signatureCacheKey(queryHash, operationName);
Expand All @@ -559,20 +556,20 @@ export class EngineReportingAgent<TContext = any> {
return cachedSignature;
}

if (!documentAST) {
if (!document) {
// 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
// hides literals, you might end up sending literals for queries
// that fail parsing or validation. Provide some way to mask them
// anyway?
return queryString as string;
return source as string;
}

const generatedSignature = (
this.options.calculateSignature || defaultEngineReportingSignature
)(documentAST, operationName);
)(document, operationName);

// Intentionally not awaited so the cache can be written to at leisure.
this.signatureCache.set(cacheKey, generatedSignature);
Expand Down