Skip to content

ref(node): Streamline dataloader instrumentation#21475

Open
mydea wants to merge 4 commits into
developfrom
fn/streamline-dataloader
Open

ref(node): Streamline dataloader instrumentation#21475
mydea wants to merge 4 commits into
developfrom
fn/streamline-dataloader

Conversation

@mydea

@mydea mydea commented Jun 11, 2026

Copy link
Copy Markdown
Member

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

@mydea mydea requested review from andreiborza and nicohrubec June 11, 2026 15:18
@mydea mydea self-assigned this Jun 11, 2026
@mydea mydea requested a review from a team as a code owner June 11, 2026 15:18
@mydea mydea requested review from JPeer264 and removed request for a team June 11, 2026 15:18
@mydea mydea force-pushed the fn/streamline-dataloader branch from f1ab607 to 070c3c0 Compare June 12, 2026 07:33

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 070c3c0. Configure here.

await runner.makeRequest('get', '/load');
await runner.makeRequest('get', '/load-many');
await runner.makeRequest('get', '/cache-ops');
await runner.makeRequest('get', '/named');

Copy link
Copy Markdown

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 with expectedEnvelopes.shift(), so the first callback always runs on the first transaction received—not necessarily GET /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.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 070c3c0. Configure here.

Copy link
Copy Markdown
Member Author

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

@mydea mydea force-pushed the fn/streamline-dataloader branch from 946b207 to 837e1db Compare June 12, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streamline @opentelemetry/instrumentation-dataloader

1 participant