Skip to content

Commit 1bf2bad

Browse files
mydeamydea
authored
ref(node): Streamline dataloader instrumentation (#21475)
This streamlines the vendored dataloader instrumentation: * Use `Sentry.startSpan` instead of tracer APIs * this allows us to streamline a bunch of things, e.g. using the built-in onlyIfParent option * Remove instrumentation options we hardcode anyhow (we always use onlyIfParent) * Streamline types to what we actually need * Fix linting errors (remove eslint-disable comment) * Add tests covering all methods, actually * Moved custom span attributes etc. we had to hackily set in `on('spanStart')` directly into the instrumentation This is overall much simpler, as we use the built-in error handling etc. from Sentry.startSpan. Closes #20725
1 parent 0a6e864 commit 1bf2bad

7 files changed

Lines changed: 277 additions & 305 deletions

File tree

‎.oxlintrc.base.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,6 @@
142142
},
143143
{
144144
"files": [
145-
"**/integrations/tracing/dataloader/vendored/**/*.ts",
146145
"**/integrations/fs/vendored/**/*.ts",
147146
"**/integrations/tracing/knex/vendored/**/*.ts",
148147
"**/integrations/tracing/mongo/vendored/**/*.ts",

‎dev-packages/node-integration-tests/suites/tracing/dataloader/scenario.mjs‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,35 @@ import express from 'express';
44

55
const PORT = 8008;
66

7+
const batchLoadFn = async keys => keys.map((_, idx) => idx);
8+
79
const run = async () => {
810
const app = express();
9-
const dataloader = new Dataloader(async keys => keys.map((_, idx) => idx), {
10-
cache: false,
11+
12+
app.get('/load', async (_req, res) => {
13+
const dataloader = new Dataloader(batchLoadFn, { cache: false });
14+
const user = await dataloader.load('user-1');
15+
res.send({ user });
16+
});
17+
18+
app.get('/load-many', async (_req, res) => {
19+
const dataloader = new Dataloader(batchLoadFn, { cache: false });
20+
const users = await dataloader.loadMany(['user-1', 'user-2']);
21+
res.send({ users });
22+
});
23+
24+
app.get('/cache-ops', async (_req, res) => {
25+
const dataloader = new Dataloader(batchLoadFn);
26+
dataloader.prime('user-1', 1);
27+
dataloader.clear('user-1');
28+
dataloader.clearAll();
29+
res.send({});
1130
});
1231

13-
app.get('/', (req, res) => {
14-
const user = dataloader.load('user-1');
15-
res.send(user);
32+
app.get('/named', async (_req, res) => {
33+
const dataloader = new Dataloader(batchLoadFn, { cache: false, name: 'usersLoader' });
34+
const user = await dataloader.load('user-1');
35+
res.send({ user });
1636
});
1737

1838
startExpressServerAndSendPortToRunner(app, PORT);

‎dev-packages/node-integration-tests/suites/tracing/dataloader/test.ts‎

Lines changed: 85 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,96 @@
11
import { afterAll, describe, expect } from 'vitest';
22
import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner';
33

4+
const ORIGIN = 'auto.db.otel.dataloader';
5+
const CACHE_GET_OP = 'cache.get';
6+
47
describe('dataloader auto-instrumentation', () => {
5-
afterAll(async () => {
8+
afterAll(() => {
69
cleanupChildProcesses();
710
});
811

9-
const EXPECTED_TRANSACTION = {
10-
transaction: 'GET /',
11-
spans: expect.arrayContaining([
12-
expect.objectContaining({
13-
data: expect.objectContaining({
14-
'sentry.origin': 'auto.db.otel.dataloader',
15-
'sentry.op': 'cache.get',
16-
}),
17-
description: 'dataloader.load',
18-
origin: 'auto.db.otel.dataloader',
19-
op: 'cache.get',
20-
status: 'ok',
21-
}),
22-
expect.objectContaining({
23-
data: expect.objectContaining({
24-
'sentry.origin': 'auto.db.otel.dataloader',
25-
'sentry.op': 'cache.get',
26-
}),
27-
description: 'dataloader.batch',
28-
origin: 'auto.db.otel.dataloader',
29-
op: 'cache.get',
30-
status: 'ok',
31-
}),
32-
]),
33-
};
34-
3512
createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {
36-
test('should auto-instrument `dataloader` package.', async () => {
37-
const runner = createRunner().expect({ transaction: EXPECTED_TRANSACTION }).start();
38-
runner.makeRequest('get', '/');
13+
test('instruments load, loadMany, batch, prime, clear and clearAll', async () => {
14+
const runner = createRunner()
15+
.expect({
16+
transaction: event => {
17+
expect(event.transaction).toBe('GET /load');
18+
19+
const spans = event.spans || [];
20+
21+
const loadSpan = spans.find(span => span.description === 'dataloader.load');
22+
expect(loadSpan).toBeDefined();
23+
expect(loadSpan?.op).toBe(CACHE_GET_OP);
24+
expect(loadSpan?.origin).toBe(ORIGIN);
25+
expect(loadSpan?.status).toBe('ok');
26+
expect(loadSpan?.data?.['sentry.origin']).toBe(ORIGIN);
27+
expect(loadSpan?.data?.['sentry.op']).toBe(CACHE_GET_OP);
28+
29+
const batchSpan = spans.find(span => span.description === 'dataloader.batch');
30+
expect(batchSpan).toBeDefined();
31+
expect(batchSpan?.op).toBe(CACHE_GET_OP);
32+
expect(batchSpan?.origin).toBe(ORIGIN);
33+
expect(batchSpan?.status).toBe('ok');
34+
35+
// The batch span links back to the load span that triggered it
36+
expect(batchSpan?.links).toEqual([
37+
expect.objectContaining({
38+
trace_id: loadSpan?.trace_id,
39+
span_id: loadSpan?.span_id,
40+
}),
41+
]);
42+
},
43+
})
44+
.expect({
45+
transaction: event => {
46+
expect(event.transaction).toBe('GET /load-many');
47+
48+
const loadManySpan = (event.spans || []).find(span => span.description === 'dataloader.loadMany');
49+
expect(loadManySpan).toBeDefined();
50+
expect(loadManySpan?.op).toBe(CACHE_GET_OP);
51+
expect(loadManySpan?.origin).toBe(ORIGIN);
52+
expect(loadManySpan?.status).toBe('ok');
53+
expect(loadManySpan?.data?.['sentry.origin']).toBe(ORIGIN);
54+
expect(loadManySpan?.data?.['sentry.op']).toBe(CACHE_GET_OP);
55+
},
56+
})
57+
.expect({
58+
transaction: event => {
59+
expect(event.transaction).toBe('GET /cache-ops');
60+
61+
const spans = event.spans || [];
62+
63+
// prime/clear/clearAll are not cache reads, so they get an origin but no `op`
64+
for (const operation of ['prime', 'clear', 'clearAll']) {
65+
const span = spans.find(s => s.description === `dataloader.${operation}`);
66+
expect(span, `expected a dataloader.${operation} span`).toBeDefined();
67+
expect(span?.origin).toBe(ORIGIN);
68+
expect(span?.status).toBe('ok');
69+
expect(span?.op).toBeUndefined();
70+
expect(span?.data?.['sentry.origin']).toBe(ORIGIN);
71+
expect(span?.data?.['sentry.op']).toBeUndefined();
72+
}
73+
},
74+
})
75+
.expect({
76+
transaction: event => {
77+
expect(event.transaction).toBe('GET /named');
78+
79+
// A named dataloader includes its name in the span description
80+
const namedLoadSpan = (event.spans || []).find(span => span.description === 'dataloader.load usersLoader');
81+
expect(namedLoadSpan).toBeDefined();
82+
expect(namedLoadSpan?.op).toBe(CACHE_GET_OP);
83+
expect(namedLoadSpan?.origin).toBe(ORIGIN);
84+
expect(namedLoadSpan?.status).toBe('ok');
85+
},
86+
})
87+
.start();
88+
89+
await runner.makeRequest('get', '/load');
90+
await runner.makeRequest('get', '/load-many');
91+
await runner.makeRequest('get', '/cache-ops');
92+
await runner.makeRequest('get', '/named');
3993
await runner.completed();
40-
});
94+
}, 30_000);
4195
});
4296
});

‎packages/node/src/integrations/tracing/dataloader/index.ts‎

Lines changed: 4 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,17 @@
11
import { DataloaderInstrumentation } from './vendored/instrumentation';
22
import type { IntegrationFn } from '@sentry/core';
3-
import {
4-
defineIntegration,
5-
SEMANTIC_ATTRIBUTE_SENTRY_OP,
6-
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
7-
spanToJSON,
8-
} from '@sentry/core';
9-
import { generateInstrumentOnce, instrumentWhenWrapped } from '@sentry/node-core';
3+
import { defineIntegration } from '@sentry/core';
4+
import { generateInstrumentOnce } from '@sentry/node-core';
105

116
const INTEGRATION_NAME = 'Dataloader';
127

13-
export const instrumentDataloader = generateInstrumentOnce(
14-
INTEGRATION_NAME,
15-
() =>
16-
new DataloaderInstrumentation({
17-
requireParentSpan: true,
18-
}),
19-
);
8+
export const instrumentDataloader = generateInstrumentOnce(INTEGRATION_NAME, () => new DataloaderInstrumentation());
209

2110
const _dataloaderIntegration = (() => {
22-
let instrumentationWrappedCallback: undefined | ((callback: () => void) => void);
23-
2411
return {
2512
name: INTEGRATION_NAME,
2613
setupOnce() {
27-
const instrumentation = instrumentDataloader();
28-
instrumentationWrappedCallback = instrumentWhenWrapped(instrumentation);
29-
},
30-
31-
setup(client) {
32-
// This is called either immediately or when the instrumentation is wrapped
33-
instrumentationWrappedCallback?.(() => {
34-
client.on('spanStart', span => {
35-
const spanJSON = spanToJSON(span);
36-
if (spanJSON.description?.startsWith('dataloader')) {
37-
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.db.otel.dataloader');
38-
}
39-
40-
// These are all possible dataloader span descriptions
41-
// Still checking for the future versions
42-
// in case they add support for `clear` and `prime`
43-
if (
44-
spanJSON.description === 'dataloader.load' ||
45-
spanJSON.description === 'dataloader.loadMany' ||
46-
spanJSON.description === 'dataloader.batch'
47-
) {
48-
span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'cache.get');
49-
// TODO: We can try adding `key` to the `data` attribute upstream.
50-
// Or alternatively, we can add `requestHook` to the dataloader instrumentation.
51-
}
52-
});
53-
});
14+
instrumentDataloader();
5415
},
5516
};
5617
}) satisfies IntegrationFn;

‎packages/node/src/integrations/tracing/dataloader/vendored/dataloader-types.ts‎

Lines changed: 0 additions & 19 deletions
This file was deleted.

0 commit comments

Comments
 (0)