Skip to content

Commit

Permalink
Surface logger within getTraceSignature to "catch" & log cache er…
Browse files Browse the repository at this point in the history
…rors.

While a failure to write to the cache in its current state would be unlikely
since it's tied to an `InMemoryCacheLRU`, we should be able to have this
cache backed by a distributed store (e.g., Memcached, Redis) in the future,
where these writes _could_ fail.
  • Loading branch information
abernix committed May 20, 2020
1 parent 500cc3f commit 5cf2986
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 1 deletion.
33 changes: 32 additions & 1 deletion packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ export interface AddTraceArgs {
executableSchemaId: string;
source?: string;
document?: DocumentNode;
logger: Logger,
}

const serviceHeaderDefaults = {
Expand Down Expand Up @@ -488,6 +489,16 @@ export class EngineReportingAgent<TContext = any> {
operationName,
source,
executableSchemaId,
/**
* Since this agent instruments the plugin with its `options.logger`, but
* also passes off a reference to this `addTrace` method which is invoked
* with the availability of a per-request `logger`, this `logger` (in this
* destructuring) is already conditionally either:
*
* 1. The `logger` that was passed into the `options` for the agent.
* 2. The request-specific `logger`.
*/
logger,
}: AddTraceArgs): Promise<void> {
// Ignore traces that come in after stop().
if (this.stopped) {
Expand All @@ -508,6 +519,7 @@ export class EngineReportingAgent<TContext = any> {
document,
source,
operationName,
logger,
});

const statsReportKey = `# ${operationName || '-'}\n${signature}`;
Expand Down Expand Up @@ -729,11 +741,13 @@ export class EngineReportingAgent<TContext = any> {
operationName,
document,
source,
logger,
}: {
queryHash: string;
operationName: string;
document?: DocumentNode;
source?: string;
logger: Logger;
}): Promise<string> {
if (!document && !source) {
// This shouldn't happen: one of those options must be passed to runQuery.
Expand Down Expand Up @@ -768,7 +782,24 @@ export class EngineReportingAgent<TContext = any> {
)(document, operationName);

// Intentionally not awaited so the cache can be written to at leisure.
this.signatureCache.set(cacheKey, generatedSignature);
//
// As of the writing of this comment, this signature cache is exclusively
// backed by an `InMemoryLRUCache` which cannot do anything
// non-synchronously, though that will probably change in the future,
// and a distributed cache store, like Redis, doesn't seem unfathomable.
//
// Due in part to the plugin being separate from the `EngineReportingAgent`,
// the loggers are difficult to track down here. Errors will be logged to
// either the request-specific logger on the request context (if available)
// or to the `logger` that was passed into `EngineReportingOptions` which
// is provided in the `EngineReportingAgent` constructor options.
this.signatureCache.set(cacheKey, generatedSignature)
.catch(err => {
logger.warn(
'Could not store signature cache. ' +
(err && err.message) || err
)
});

return generatedSignature;
}
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-engine-reporting/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export const plugin = <TContext>(
executableSchemaId: executableSchemaIdGenerator(
options.experimental_overrideReportedSchema || schema,
),
logger,
});
}

Expand Down

0 comments on commit 5cf2986

Please sign in to comment.