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

feat(instrumentation-dataloader): Enhance dataloader instrumentation. #2449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
159 changes: 148 additions & 11 deletions plugins/node/instrumentation-dataloader/src/instrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@
type DataloaderInternal = typeof Dataloader.prototype & {
_batchLoadFn: Dataloader.BatchLoadFn<unknown, unknown>;
_batch: { spanLinks?: Link[] } | null;

// TODO: Remove this once types on Dataloader get fixed https://github.com/graphql/dataloader/pull/334
name?: string | null;
};

type LoadFn = (typeof Dataloader.prototype)['load'];
type LoadManyFn = (typeof Dataloader.prototype)['loadMany'];
type PrimeFn = (typeof Dataloader.prototype)['prime'];
type ClearFn = (typeof Dataloader.prototype)['clear'];
type ClearAllFn = (typeof Dataloader.prototype)['clearAll'];

export class DataloaderInstrumentation extends InstrumentationBase<DataloaderInstrumentationConfig> {
constructor(config: DataloaderInstrumentationConfig = {}) {
Expand All @@ -56,17 +56,18 @@
dataloader => {
this._patchLoad(dataloader.prototype);
this._patchLoadMany(dataloader.prototype);
this._patchPrime(dataloader.prototype);
this._patchClear(dataloader.prototype);
this._patchClearAll(dataloader.prototype);

return this._getPatchedConstructor(dataloader);
},
dataloader => {
if (isWrapped(dataloader.prototype.load)) {
this._unwrap(dataloader.prototype, 'load');
}

if (isWrapped(dataloader.prototype.loadMany)) {
this._unwrap(dataloader.prototype, 'loadMany');
}
['load', 'loadMany', 'prime', 'clear', 'clearAll'].forEach(method => {
if (isWrapped(dataloader.prototype[method])) {
this._unwrap(dataloader.prototype, method);
}
});
}
) as InstrumentationNodeModuleDefinition,
];
Expand All @@ -80,7 +81,7 @@

private getSpanName(
dataloader: DataloaderInternal,
operation: 'load' | 'loadMany' | 'batch'
operation: 'load' | 'loadMany' | 'batch' | 'prime' | 'clear' | 'clearAll'
): string {
const dataloaderName = dataloader.name;
if (dataloaderName === undefined || dataloaderName === null) {
Expand Down Expand Up @@ -184,6 +185,13 @@
const result = original
.call(this, ...args)
.then(value => {
span.setAttribute('cache.key', [args[0]]);
span.setAttribute('cache.hit', value !== undefined);
span.setAttribute(
'cache.item_size',
value ? JSON.stringify(value).length : 0
);

span.end();
return value;
})
Expand Down Expand Up @@ -242,10 +250,139 @@
// .loadMany never rejects, as errors from internal .load
// calls are caught by dataloader lib
return original.call(this, ...args).then(value => {
span.setAttribute('cache.key', Array.from(args[0]));
span.setAttribute(
'cache.hit',
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately these are not official OTEL semantic conventions, so I think we cannot add these here. I think we'll need requestHook and add these ourselves. For now we should remove this.

There are no cache conventions for OTEL yet. We're thinking about donating our conventions upstream for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AbhiPrasad - Should we add the requestHook with these updates, or in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Let's add requestHook in a separate PR

value.every((v: unknown) => v !== undefined)
);
span.setAttribute(
'cache.item_size',
value.reduce(
(acc: number, v: unknown) => acc + JSON.stringify(v).length,
0
)
);

span.end();
return value;
});
});
};
}

private _patchPrime(proto: typeof Dataloader.prototype) {
if (isWrapped(proto.prime)) {
this._unwrap(proto, 'prime');
}

this._wrap(proto, 'prime', this._getPatchedPrime.bind(this));
}

private _getPatchedPrime(original: PrimeFn): PrimeFn {
const instrumentation = this;

return function patchedPrime(
this: DataloaderInternal,
...args: Parameters<typeof original>
) {
if (!instrumentation.shouldCreateSpans()) {
return original.call(this, ...args);

Check warning on line 289 in plugins/node/instrumentation-dataloader/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/instrumentation-dataloader/src/instrumentation.ts#L289

Added line #L289 was not covered by tests
}

const parent = context.active();
const span = instrumentation.tracer.startSpan(
instrumentation.getSpanName(this, 'prime'),
{ kind: SpanKind.CLIENT },
parent
);

const ret = context.with(trace.setSpan(parent, span), () => {
return original.call(this, ...args);
});

span.setAttribute('cache.key', [args[0]]);
span.setAttribute(
'cache.item_size',
args[1] ? JSON.stringify(args[1]).length : 0
);

span.end();

return ret;
};
}

private _patchClear(proto: typeof Dataloader.prototype) {
if (isWrapped(proto.clear)) {
this._unwrap(proto, 'clear');
}

this._wrap(proto, 'clear', this._getPatchedClear.bind(this));
}

private _getPatchedClear(original: ClearFn): ClearFn {
const instrumentation = this;

return function patchedClear(
this: DataloaderInternal,
...args: Parameters<typeof original>
) {
if (!instrumentation.shouldCreateSpans()) {
return original.call(this, ...args);

Check warning on line 331 in plugins/node/instrumentation-dataloader/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/instrumentation-dataloader/src/instrumentation.ts#L331

Added line #L331 was not covered by tests
}

const parent = context.active();
const span = instrumentation.tracer.startSpan(
instrumentation.getSpanName(this, 'clear'),
{ kind: SpanKind.CLIENT },
parent
);

const ret = context.with(trace.setSpan(parent, span), () => {
span.setAttribute('cache.key', [args[0]]);

return original.call(this, ...args);
});

span.end();

return ret;
};
}

private _patchClearAll(proto: typeof Dataloader.prototype) {
if (isWrapped(proto.clearAll)) {
this._unwrap(proto, 'clearAll');
}

this._wrap(proto, 'clearAll', this._getPatchedClearAll.bind(this));
}

private _getPatchedClearAll(original: ClearAllFn): ClearAllFn {
const instrumentation = this;

return function patchedClearAll(
this: DataloaderInternal,
...args: Parameters<typeof original>
) {
if (!instrumentation.shouldCreateSpans()) {
return original.call(this, ...args);

Check warning on line 369 in plugins/node/instrumentation-dataloader/src/instrumentation.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/instrumentation-dataloader/src/instrumentation.ts#L369

Added line #L369 was not covered by tests
}

const parent = context.active();
const span = instrumentation.tracer.startSpan(
instrumentation.getSpanName(this, 'clearAll'),
{ kind: SpanKind.CLIENT },
parent
);

const ret = context.with(trace.setSpan(parent, span), () => {
return original.call(this, ...args);
});

span.end();

return ret;
};
}
}
Loading