Provide ability to specify client info in traces #1631
Conversation
ghost
added
the
evans
force-pushed
the
evans/client-id
branch
from
c9a415d
to
7cf65b5
Compare
docs/source/api/apollo-server.md
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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?
| createClientInfo?: ( | ||
| o: { | ||
| request: Request; | ||
| queryString?: string; |
There was a problem hiding this comment.
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.
| @@ -34,6 +34,12 @@ Traces.encode = function(message, originalWriter) { | |||
| return writer; | |||
| }; | |||
|
|
|||
| export interface ClientInfo { | |||
| clientName?: string; | |||
| clientAddress?: string; | |||
There was a problem hiding this comment.
I think we can leave out clientAddress and clientVersion for now
evans
force-pushed
the
evans/client-id
branch
from
b4d5f8e
to
342fda8
Compare
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.
evans
force-pushed
the
evans/client-id
branch
from
342fda8
to
b43cdd3
Compare
The frontend will not support it in the near future
createClientInfo -> generateClientInfo
docs/source/api/apollo-server.md
Outdated
| 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 |
| @@ -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 | |||
There was a problem hiding this comment.
clientInfo extensions field
docs/source/api/apollo-server.md
Outdated
| 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` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I like POST body's extensions.clientInfo! Reading the sentence again, it isn't quite clear 🎉
| this.generateClientInfo = | ||
| options.generateClientInfo || | ||
| // Default to using the clientInfo field of the request | ||
| (({ extensions }) => (extensions && extensions.clientInfo) || {}); |
There was a problem hiding this comment.
What happens if clientInfo type doesn't match the object expected?
There was a problem hiding this comment.
clientName and clientVersion will be undefined
| context: o.context, | ||
| extensions: o.extensions, | ||
| }); | ||
| this.trace.clientName = clientName || ''; |
There was a problem hiding this comment.
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
* 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
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: