Provide ability to specify client info in traces #1631

evans · 2018-09-07T23:24:29Z

Adds the createClientInfo to apollo-engine-reporting, which enables the
differentiation of clients based on the request, operation, and
variables. This could be extended to include the response. However for
the first release. It doesn't quite make sense.

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

zionts · 2018-09-07T23:29:06Z

docs/source/api/apollo-server.md

@@ -348,3 +348,9 @@ addMockFunctionsToSchema({
 *  `maskErrorDetails`: boolean

   Set to true to remove error details from the traces sent to Apollo's servers. Defaults to false.
+
+* createClientInfo?: (o: { request: Request, queryString?: string, parsedQuery?: DocumentNode, variables: Record<string, any>}) => ClientInfo;


Should we have a default createClientInfo function that looks to a known header or set of headers for these fields? Also, I think the naming could be better, since this is actually a function that takes in request details and generates the client information. Maybe clientInfoGenerator?

zionts · 2018-09-07T23:31:28Z

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

+  createClientInfo?: (
+    o: {
+      request: Request;
+      queryString?: string;


I'm confused as to why we're passing the query information here? I can understand using the variables as well as query extensions, but I don't understand the query string.

zionts · 2018-09-07T23:38:05Z

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

@@ -34,6 +34,12 @@ Traces.encode = function(message, originalWriter) {
  return writer;
 };

+export interface ClientInfo {
+  clientName?: string;
+  clientAddress?: string;


I think we can leave out clientAddress and clientVersion for now

Adds the createClientInfo to apollo-engine-reporting, which enables the differentiation of clients based on the request, operation, and variables. This could be extended to include the response. However for the first release. It doesn't quite make sense.

The frontend will not support it in the near future

createClientInfo -> generateClientInfo

zionts

Looks good! Just some small comment nitpicks and an ask about type coercion and default values.

zionts · 2018-09-18T05:07:31Z

docs/source/api/apollo-server.md

+   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


can both be null?

zionts · 2018-09-18T05:08:19Z

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

@@ -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


clientInfo extensions field

zionts · 2018-09-18T05:09:48Z

docs/source/api/apollo-server.md

+   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
+   value copies the respective fields from the `extensions`'s `clientInfo`


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?

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

zionts · 2018-09-18T05:10:23Z

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

+    this.generateClientInfo =
+      options.generateClientInfo ||
+      // Default to using the clientInfo field of the request
+      (({ extensions }) => (extensions && extensions.clientInfo) || {});


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

clientName and clientVersion will be undefined

zionts · 2018-09-18T05:12:12Z

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

+      context: o.context,
+      extensions: o.extensions,
+    });
+    this.trace.clientName = clientName || '';


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?

The `generateClientInfo` API, used to set client identification attributes within traces, is an experimental API and is subject to removal or change in a future (major) Apollo Server release. Ref: #1631

This reverts commit 96af44e.

* Provide ability to specify client info in traces Adds the createClientInfo to apollo-engine-reporting, which enables the differentiation of clients based on the request, operation, and variables. This could be extended to include the response. However for the first release. It doesn't quite make sense. * Use extensions and context in createClientInfo * Remove support for clientAddress The frontend will not support it in the near future * create -> generate and make default generator createClientInfo -> generateClientInfo * Clarify default values

The `generateClientInfo` API, used to set client identification attributes within traces, is an experimental API and is subject to removal or change in a future (major) Apollo Server release. Ref: #1631

ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 7, 2018

evans force-pushed the evans/client-id branch from c9a415d to 7cf65b5 Compare September 7, 2018 23:25

ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 7, 2018

zionts reviewed Sep 7, 2018

View reviewed changes

evans force-pushed the evans/client-id branch from b4d5f8e to 342fda8 Compare September 17, 2018 21:40

evans force-pushed the evans/client-id branch from 342fda8 to b43cdd3 Compare September 18, 2018 01:23

evans added 3 commits September 17, 2018 21:27

Use extensions and context in createClientInfo

fd3f5de

Remove support for clientAddress

adae6f7

The frontend will not support it in the near future

create -> generate and make default generator

a6328b9

createClientInfo -> generateClientInfo

zionts reviewed Sep 18, 2018

View reviewed changes

zionts approved these changes Sep 18, 2018

View reviewed changes

Clarify default values

843fd64

evans merged commit 96af44e into master Sep 18, 2018

evans deleted the evans/client-id branch September 18, 2018 05:45

dependencies bot mentioned this pull request Sep 19, 2018

Update apollo-server-express in / from 1.2.0 to 2.1.0 LibreTubeApp/LibreTube#192

Open

abernix added a commit that referenced this pull request Sep 27, 2018

Revert "Provide ability to specify client info in traces (#1631)"

68c82e6

This reverts commit 96af44e.

dependencies bot mentioned this pull request Oct 1, 2018

Update apollo-server-express in /modules/api from 1.3.4 to 2.1.0 project-accelerate/accelerate#241

Open

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide ability to specify client info in traces #1631

Provide ability to specify client info in traces #1631

evans commented Sep 7, 2018 •

edited

Loading

zionts Sep 7, 2018

zionts Sep 7, 2018

zionts Sep 7, 2018

zionts left a comment

zionts Sep 18, 2018

zionts Sep 18, 2018

zionts Sep 18, 2018

evans Sep 18, 2018

zionts Sep 18, 2018

evans Sep 18, 2018

zionts Sep 18, 2018

Provide ability to specify client info in traces #1631

Provide ability to specify client info in traces #1631

Conversation

evans commented Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zionts 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

Choose a reason for hiding this comment

evans commented Sep 7, 2018 •

edited

Loading