Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
ref(node): Streamline dataloader instrumentation#21475
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,42 +1,96 @@ | ||
| import { afterAll, describe, expect } from 'vitest'; | ||
| import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; | ||
| const ORIGIN = 'auto.db.otel.dataloader'; | ||
| const CACHE_GET_OP = 'cache.get'; | ||
| describe('dataloader auto-instrumentation', () => { | ||
| afterAll(async () => { | ||
| afterAll(() => { | ||
| cleanupChildProcesses(); | ||
| }); | ||
| const EXPECTED_TRANSACTION = { | ||
| transaction: 'GET /', | ||
| spans: expect.arrayContaining([ | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.otel.dataloader', | ||
| 'sentry.op': 'cache.get', | ||
| }), | ||
| description: 'dataloader.load', | ||
| origin: 'auto.db.otel.dataloader', | ||
| op: 'cache.get', | ||
| status: 'ok', | ||
| }), | ||
| expect.objectContaining({ | ||
| data: expect.objectContaining({ | ||
| 'sentry.origin': 'auto.db.otel.dataloader', | ||
| 'sentry.op': 'cache.get', | ||
| }), | ||
| description: 'dataloader.batch', | ||
| origin: 'auto.db.otel.dataloader', | ||
| op: 'cache.get', | ||
| status: 'ok', | ||
| }), | ||
| ]), | ||
| }; | ||
| createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { | ||
| test('should auto-instrument `dataloader` package.', async () => { | ||
| const runner = createRunner().expect({ transaction: EXPECTED_TRANSACTION }).start(); | ||
| runner.makeRequest('get', '/'); | ||
| test('instruments load, loadMany, batch, prime, clear and clearAll', async () => { | ||
| const runner = createRunner() | ||
| .expect({ | ||
| transaction: event => { | ||
| expect(event.transaction).toBe('GET /load'); | ||
| const spans = event.spans || []; | ||
| const loadSpan = spans.find(span => span.description === 'dataloader.load'); | ||
| expect(loadSpan).toBeDefined(); | ||
| expect(loadSpan?.op).toBe(CACHE_GET_OP); | ||
| expect(loadSpan?.origin).toBe(ORIGIN); | ||
| expect(loadSpan?.status).toBe('ok'); | ||
| expect(loadSpan?.data?.['sentry.origin']).toBe(ORIGIN); | ||
| expect(loadSpan?.data?.['sentry.op']).toBe(CACHE_GET_OP); | ||
| const batchSpan = spans.find(span => span.description === 'dataloader.batch'); | ||
| expect(batchSpan).toBeDefined(); | ||
| expect(batchSpan?.op).toBe(CACHE_GET_OP); | ||
| expect(batchSpan?.origin).toBe(ORIGIN); | ||
| expect(batchSpan?.status).toBe('ok'); | ||
| // The batch span links back to the load span that triggered it | ||
| expect(batchSpan?.links).toEqual([ | ||
| expect.objectContaining({ | ||
| trace_id: loadSpan?.trace_id, | ||
| span_id: loadSpan?.span_id, | ||
| }), | ||
| ]); | ||
| }, | ||
| }) | ||
| .expect({ | ||
| transaction: event => { | ||
| expect(event.transaction).toBe('GET /load-many'); | ||
| const loadManySpan = (event.spans || []).find(span => span.description === 'dataloader.loadMany'); | ||
| expect(loadManySpan).toBeDefined(); | ||
| expect(loadManySpan?.op).toBe(CACHE_GET_OP); | ||
| expect(loadManySpan?.origin).toBe(ORIGIN); | ||
| expect(loadManySpan?.status).toBe('ok'); | ||
| expect(loadManySpan?.data?.['sentry.origin']).toBe(ORIGIN); | ||
| expect(loadManySpan?.data?.['sentry.op']).toBe(CACHE_GET_OP); | ||
| }, | ||
| }) | ||
| .expect({ | ||
| transaction: event => { | ||
| expect(event.transaction).toBe('GET /cache-ops'); | ||
| const spans = event.spans || []; | ||
| // prime/clear/clearAll are not cache reads, so they get an origin but no `op` | ||
| for (const operation of ['prime', 'clear', 'clearAll']) { | ||
| const span = spans.find(s => s.description === `dataloader.${operation}`); | ||
| expect(span, `expected a dataloader.${operation} span`).toBeDefined(); | ||
| expect(span?.origin).toBe(ORIGIN); | ||
| expect(span?.status).toBe('ok'); | ||
| expect(span?.op).toBeUndefined(); | ||
| expect(span?.data?.['sentry.origin']).toBe(ORIGIN); | ||
| expect(span?.data?.['sentry.op']).toBeUndefined(); | ||
| } | ||
| }, | ||
| }) | ||
| .expect({ | ||
| transaction: event => { | ||
| expect(event.transaction).toBe('GET /named'); | ||
| // A named dataloader includes its name in the span description | ||
| const namedLoadSpan = (event.spans || []).find(span => span.description === 'dataloader.load usersLoader'); | ||
| expect(namedLoadSpan).toBeDefined(); | ||
| expect(namedLoadSpan?.op).toBe(CACHE_GET_OP); | ||
| expect(namedLoadSpan?.origin).toBe(ORIGIN); | ||
| expect(namedLoadSpan?.status).toBe('ok'); | ||
| }, | ||
| }) | ||
| .start(); | ||
| await runner.makeRequest('get', '/load'); | ||
| await runner.makeRequest('get', '/load-many'); | ||
| await runner.makeRequest('get', '/cache-ops'); | ||
| await runner.makeRequest('get', '/named'); | ||
| await runner.completed(); | ||
| }); | ||
| }, 30_000); | ||
| }); | ||
| }); | ||
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chained expects assume envelope order
Medium Severity
The test chains four
.expect({ transaction: … })callbacks, but the integration runner matches incoming envelopes withexpectedEnvelopes.shift(), so the first callback always runs on the first transaction received—not necessarilyGET /load. Four back-to-back HTTP requests can emit transactions in a different order than the expects, causing flaky failures when the wrong callback validates the wrong route.Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 070c3c0. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be possible to flush in a different order