feat(node): Add `dataloader` integration #13664

onurtemizkan · 2024-09-11T04:18:03Z

Resolves: #13313

Adds integration for dataloader using @opentelemetry/instrumentation-dataloader on the background.

A few notes:

We currently don't have access to the lookup / request as there is no hook from @opentelemetry/instrumentation-dataloader. So, we don't have cache.hit, cache.key, cache.item_size and so on, in this integration. I can try to implement those upstream, but if you have another way in mind to access those please let me know.
@opentelemetry/instrumentation-dataloader only records spans for load, loadMany and batch, which all are cache.get operations. There are also prime, clear, clearAll. We also can implement those upstream and update the integration in future.

github-actions · 2024-09-11T04:33:50Z

size-limit report 📦

Path	Size	% Change	Change
@sentry/browser	22.52 KB	-	-
@sentry/browser - with treeshaking flags	21.3 KB	-	-
@sentry/browser (incl. Tracing)	34.8 KB	-	-
@sentry/browser (incl. Tracing, Replay)	71.26 KB	-	-
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	61.7 KB	-	-
@sentry/browser (incl. Tracing, Replay with Canvas)	75.61 KB	-	-
@sentry/browser (incl. Tracing, Replay, Feedback)	88.39 KB	-	-
@sentry/browser (incl. Tracing, Replay, Feedback, metrics)	90.23 KB	-	-
@sentry/browser (incl. metrics)	26.83 KB	-	-
@sentry/browser (incl. Feedback)	39.66 KB	-	-
@sentry/browser (incl. sendFeedback)	27.19 KB	-	-
@sentry/browser (incl. FeedbackAsync)	31.96 KB	-	-
@sentry/react	25.28 KB	-	-
@sentry/react (incl. Tracing)	37.77 KB	-	-
@sentry/vue	26.72 KB	-	-
@sentry/vue (incl. Tracing)	36.67 KB	-	-
@sentry/svelte	22.66 KB	-	-
CDN Bundle	23.83 KB	-	-
CDN Bundle (incl. Tracing)	36.56 KB	-	-
CDN Bundle (incl. Tracing, Replay)	71.02 KB	-	-
CDN Bundle (incl. Tracing, Replay, Feedback)	76.33 KB	-	-
CDN Bundle - uncompressed	69.81 KB	-	-
CDN Bundle (incl. Tracing) - uncompressed	108.44 KB	-	-
CDN Bundle (incl. Tracing, Replay) - uncompressed	220.21 KB	-	-
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	233.43 KB	-	-
@sentry/nextjs (client)	37.53 KB	-	-
@sentry/sveltekit (client)	35.37 KB	-	-
@sentry/node	121.77 KB	+0.56%	+685 B 🔺
@sentry/node - without tracing	93.41 KB	-0.01%	-1 B 🔽
@sentry/aws-serverless	103.11 KB	-0.01%	-1 B 🔽

View base workflow run

AbhiPrasad

Let's create a GH issue in this repo to track the follow-up improvement to dataloader. We can then create GH issues in the OTEL repo accordingly.

Could you also link some example traces that have dataloader spans?

AbhiPrasad · 2024-09-13T09:05:42Z

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

+
+const INTEGRATION_NAME = 'Dataloader';
+
+export const instrumentDataloader = generateInstrumentOnce(INTEGRATION_NAME, () => new DataloaderInstrumentation({}));


I think we should set requireParentSpan to be true by default, and then expose config to change it accordingly, but I'm open to other opinions.

@mydea what do you think?

Let's set requireParentSpan to be true by default @onurtemizkan. We don't need to expose config for now, we can add that later based on user request.

Just realised it's also true by default in DataloaderIntegration. Still, passed true from our side, and we can expose it later if we need to.

this sounds reasonable to me, I guess you rarely want this to be a transaction!

onurtemizkan · 2024-09-19T08:21:58Z

Could you also link some example traces that have dataloader spans?

@AbhiPrasad, here's the trace of what we test in the integration tests. Link

AbhiPrasad

Alright let's get this in, and work toward improving stuff on it upstream!

Adds the PR link to the changelog for easier access. An entry would look like this: > - feat(node): Add `dataloader` integration ([#13664](#13664)) > ``` > - feat(node): Add `dataloader` integration ([#13664](#13664)) > ```

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.30.0` -> `8.31.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.30.0/8.31.0) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.30.0` -> `8.31.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.30.0/8.31.0) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@sentry/node)</summary> ### [`v8.31.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8310) [Compare Source](getsentry/sentry-javascript@8.30.0...8.31.0) ##### Important Changes - **feat(node): Add `dataloader` integration ([#13664](getsentry/sentry-javascript#13664 This release adds a new integration for the [`dataloader` package](https://www.npmjs.com/package/dataloader). The Node SDK (and all SDKs that depend on it) will now automatically instrument `dataloader` instances. You can also add it manually: ```js Sentry.init({ integrations: [Sentry.dataloaderIntegration()], }); ``` ##### Other Changes - feat(browser): Add navigation `activationStart` timestamp to pageload span ([#13658](getsentry/sentry-javascript#13658)) - feat(gatsby): Add optional `deleteSourcemapsAfterUpload` ([#13610](getsentry/sentry-javascript#13610)) - feat(nextjs): Give app router prefetch requests a `http.server.prefetch` op ([#13600](getsentry/sentry-javascript#13600)) - feat(nextjs): Improve Next.js serverside span data quality ([#13652](getsentry/sentry-javascript#13652)) - feat(node): Add `disableInstrumentationWarnings` option ([#13693](getsentry/sentry-javascript#13693)) - feat(nuxt): Adding `experimental_basicServerTracing` option to Nuxt module ([#13643](getsentry/sentry-javascript#13643)) - feat(nuxt): Improve logs about adding Node option 'import' ([#13726](getsentry/sentry-javascript#13726)) - feat(replay): Add `onError` callback + other small improvements to debugging ([#13721](getsentry/sentry-javascript#13721)) - feat(replay): Add experimental option to allow for a checkout every 6 minutes ([#13069](getsentry/sentry-javascript#13069)) - feat(wasm): Unconditionally parse instruction addresses ([#13655](getsentry/sentry-javascript#13655)) - fix: Ensure all logs are wrapped with `consoleSandbox` ([#13690](getsentry/sentry-javascript#13690)) - fix(browser): Try multiple options for `lazyLoadIntegration` script parent element lookup ([#13717](getsentry/sentry-javascript#13717)) - fix(feedback): Actor color applies to feedback icon ([#13702](getsentry/sentry-javascript#13702)) - fix(feedback): Fix form width on mobile devices ([#13068](getsentry/sentry-javascript#13068)) - fix(nestjs): Preserve original function name on `SentryTraced` functions ([#13684](getsentry/sentry-javascript#13684)) - fix(node): Don't overwrite local variables for re-thrown errors ([#13644](getsentry/sentry-javascript#13644)) - fix(normalize): Treat Infinity as NaN both are non-serializable numbers ([#13406](getsentry/sentry-javascript#13406)) - fix(nuxt): Use correct server output file path ([#13725](getsentry/sentry-javascript#13725)) - fix(opentelemetry): Always use active span in `Propagator.inject` ([#13381](getsentry/sentry-javascript#13381)) - fix(replay): Fixes potential out-of-order segments ([#13609](getsentry/sentry-javascript#13609)) Work in this release was contributed by [@KyGuy2002](https://github.com/KyGuy2002), [@artzhookov](https://github.com/artzhookov), and [@julianCast](https://github.com/julianCast). Thank you for your contributions! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/149 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>

feat(node): Add dataloader instrumentation

fb171b0

onurtemizkan force-pushed the onur/dataloader-integration branch from e95324d to fb171b0 Compare September 11, 2024 09:51

Add missing exports.

0c3445d

onurtemizkan changed the title ~~feat(node): Add dataloader instrumentation~~ feat(node): Add dataloader integration Sep 11, 2024

onurtemizkan requested review from mydea, AbhiPrasad and Lms24 and removed request for mydea September 11, 2024 13:07

onurtemizkan marked this pull request as ready for review September 11, 2024 13:08

AbhiPrasad reviewed Sep 13, 2024

View reviewed changes

Set requireParentSpan to true.

214c71d

onurtemizkan mentioned this pull request Sep 19, 2024

Improvements on dataloader integration #13724

Open

2 tasks

AbhiPrasad approved these changes Sep 19, 2024

View reviewed changes

AbhiPrasad merged commit 2ab7518 into develop Sep 19, 2024
130 checks passed

AbhiPrasad deleted the onur/dataloader-integration branch September 19, 2024 13:57

s1gr1d mentioned this pull request Sep 20, 2024

meta(changelog): Add PR link to changelog entry #13735

Merged

AbhiPrasad mentioned this pull request Sep 23, 2024

feat(js): Document dataloaderIntegration released with 8.31.0 getsentry/sentry-docs#11416

Merged

6 tasks

onurtemizkan mentioned this pull request Sep 24, 2024

feat(instrumentation-dataloader): Enhance dataloader instrumentation. open-telemetry/opentelemetry-js-contrib#2449

Open

michalsanger mentioned this pull request Oct 4, 2024

Broken custom dataloader methods #13869

Open

3 tasks

hiroshinishio mentioned this pull request Oct 5, 2024

Broken custom dataloader methods hiroshinishio/sentry-javascript#7

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): Add `dataloader` integration #13664

feat(node): Add `dataloader` integration #13664

onurtemizkan commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024 •

edited

Loading

AbhiPrasad left a comment

AbhiPrasad Sep 13, 2024

AbhiPrasad Sep 19, 2024

onurtemizkan Sep 19, 2024

mydea Sep 19, 2024

onurtemizkan commented Sep 19, 2024

AbhiPrasad left a comment


		const INTEGRATION_NAME = 'Dataloader';

		export const instrumentDataloader = generateInstrumentOnce(INTEGRATION_NAME, () => new DataloaderInstrumentation({}));

feat(node): Add dataloader integration #13664

feat(node): Add dataloader integration #13664

Conversation

onurtemizkan commented Sep 11, 2024 • edited Loading

github-actions bot commented Sep 11, 2024 • edited Loading

size-limit report 📦

AbhiPrasad left a comment

Choose a reason for hiding this comment

AbhiPrasad Sep 13, 2024

Choose a reason for hiding this comment

AbhiPrasad Sep 19, 2024

Choose a reason for hiding this comment

onurtemizkan Sep 19, 2024

Choose a reason for hiding this comment

mydea Sep 19, 2024

Choose a reason for hiding this comment

onurtemizkan commented Sep 19, 2024

AbhiPrasad left a comment

Choose a reason for hiding this comment

feat(node): Add `dataloader` integration #13664

feat(node): Add `dataloader` integration #13664

onurtemizkan commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024 •

edited

Loading