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

Provide ability to specify client info in traces #1631

Merged
merged 5 commits into from
Sep 18, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ All of the packages in the `apollo-server` repo are released with the same versi
### vNEXT

- Update link inside Authentication Docs [PR #1682](https://github.com/apollographql/apollo-server/pull/1682)
- Provide ability to specify client info in traces [#1631](https://github.com/apollographql/apollo-server/pull/1631)

### v2.0.8

Expand Down
10 changes: 10 additions & 0 deletions docs/source/api/apollo-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -350,3 +350,13 @@ addMockFunctionsToSchema({
* `maskErrorDetails`: boolean

Set to true to remove error details from the traces sent to Apollo's servers. Defaults to false.

* generateClientInfo?: (o: { context: any, extensions?: Record<string, any>}) => ClientInfo;

Creates the client information that is attached to the traces sent to the
Apollo backend. The context field is the execution context passed to the
resolvers and the extensions field is the field provided in the request's
query, operationName, and variables. `ClientInfo` contains fields for
`clientName` and `clientVersion`, which can both be optional. The default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can both be null?

value copies the respective fields from the `extensions`'s `clientInfo`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it clear enough to just say "the extensions's field" here, especially since query extensions are not standard? Maybe we should say the POST body's extensions.clientInfo field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like POST body's extensions.clientInfo! Reading the sentence again, it isn't quite clear 🎉

field.
13 changes: 13 additions & 0 deletions packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ Traces.encode = function(message, originalWriter) {
return writer;
};

export interface ClientInfo {
clientName?: string;
clientVersion?: string;
}

export interface EngineReportingOptions {
// API key for the service. Get this from
// [Engine](https://engine.apollographql.com) by logging in and creating
Expand Down Expand Up @@ -83,6 +88,14 @@ export interface EngineReportingOptions {
sendReportsImmediately?: boolean;
// To remove the error message from traces, set this to true. Defaults to false
maskErrorDetails?: boolean;
// Creates the client information attached to the traces sent to the Apollo
// backend
generateClientInfo?: (
o: {
context: any;
extensions?: Record<string, any>;
},
) => ClientInfo;

// XXX Provide a way to set client_name, client_version, client_address,
// service, and service_version fields. They are currently not revealed in the
Expand Down
24 changes: 23 additions & 1 deletion packages/apollo-engine-reporting/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from 'graphql-extensions';
import { Trace, google } from 'apollo-engine-reporting-protobuf';

import { EngineReportingOptions } from './agent';
import { EngineReportingOptions, ClientInfo } from './agent';
import { defaultSignature } from './signature';

// EngineReportingExtension is the per-request GraphQLExtension which creates a
Expand All @@ -38,6 +38,12 @@ export class EngineReportingExtension<TContext = any>
operationName: string,
trace: Trace,
) => void;
private generateClientInfo: (
o: {
context: any;
extensions?: Record<string, any>;
},
) => ClientInfo;

public constructor(
options: EngineReportingOptions,
Expand All @@ -51,6 +57,10 @@ export class EngineReportingExtension<TContext = any>
const root = new Trace.Node();
this.trace.root = root;
this.nodes.set(responsePathAsString(undefined), root);
this.generateClientInfo =
options.generateClientInfo ||
// Default to using the clientInfo field of the request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clientInfo extensions field

(({ extensions }) => (extensions && extensions.clientInfo) || {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if clientInfo type doesn't match the object expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clientName and clientVersion will be undefined

}

public requestDidStart(o: {
Expand All @@ -60,6 +70,8 @@ export class EngineReportingExtension<TContext = any>
variables?: Record<string, any>;
persistedQueryHit?: boolean;
persistedQueryRegister?: boolean;
context: any;
extensions?: Record<string, any>;
}): EndHandler {
this.trace.startTime = dateToTimestamp(new Date());
this.startHrTime = process.hrtime();
Expand Down Expand Up @@ -154,6 +166,16 @@ export class EngineReportingExtension<TContext = any>
});
}

// While clientAddress could be a part of the protobuf, we'll ignore it for
// now, since the backend does not group by it and Engine frontend will not
// support it in the short term
const { clientName, clientVersion } = this.generateClientInfo({
context: o.context,
extensions: o.extensions,
});
this.trace.clientName = clientName || '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do empty string for unrecognized clients or have a designated enum, like "UNKNOWN"? Either way, we should think about what happens with clients that specify their client information as the default we use for unknown clients -- is that OK?

this.trace.clientVersion = clientVersion || '';

return () => {
this.trace.durationNs = durationHrTimeToNanos(
process.hrtime(this.startHrTime),
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-core/src/runHttpQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ export async function runHttpQuery(
: false,
request: request.request,
extensions: optionsObject.extensions,
queryExtensions: extensions,
persistedQueryHit,
persistedQueryRegister,
};
Expand Down
2 changes: 2 additions & 0 deletions packages/apollo-server-core/src/runQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export interface QueryOptions {
cacheControl?: boolean | CacheControlExtensionOptions;
request: Pick<Request, 'url' | 'method' | 'headers'>;
extensions?: Array<() => GraphQLExtension>;
queryExtensions?: Record<string, any>;
persistedQueryHit?: boolean;
persistedQueryRegister?: boolean;
}
Expand Down Expand Up @@ -136,6 +137,7 @@ function doRunQuery(options: QueryOptions): Promise<GraphQLResponse> {
persistedQueryHit: options.persistedQueryHit,
persistedQueryRegister: options.persistedQueryRegister,
context,
extensions: options.queryExtensions,
});
return Promise.resolve()
.then(
Expand Down
1 change: 1 addition & 0 deletions packages/graphql-extensions/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export class GraphQLExtensionStack<TContext = any> {
persistedQueryHit?: boolean;
persistedQueryRegister?: boolean;
context: TContext;
extensions?: Record<string, any>;
}): EndHandler {
return this.handleDidStart(
ext => ext.requestDidStart && ext.requestDidStart(o),
Expand Down