test(node): Add tests for Undici #7628

AbhiPrasad · 2023-03-27T20:55:17Z

Tests for #7582!

Ended up pulling in the undici dep to make testing easier.

Tests are not as clean as I wished to be, but I'm optimizing for speed here - we can clean this up afterwards.

AbhiPrasad · 2023-03-27T20:55:47Z

packages/node/src/integrations/undici/index.ts

@@ -89,7 +93,7 @@ export class Undici implements Integration {
      const url = new URL(request.path, request.origin);
      const stringUrl = url.toString();

-      if (isSentryRequest(stringUrl)) {
+      if (isSentryRequest(stringUrl) || request.__sentry__ !== undefined) {


I kept running into this, but I'm not sure why it's happening, the spec doesn't allow for it.

Hmm this looks strange. As if the same request went twice through this channel? Anyway, I think it's fine to bail out in this case

Lms24

This looks good, thanks for adding the tests!

Lms24 · 2023-03-28T07:11:52Z

packages/node/src/integrations/undici/index.ts

+// Please note that you cannot use `console.log` to debug the callbacks registered to the `diagnostics_channel` API.
+// To debug, you can use `writeFileSync` to write to a file:
+// https://nodejs.org/api/async_hooks.html#printing-in-asynchook-callbacks
+


Sounds painful :/

Lms24 · 2023-03-28T07:12:48Z

packages/node/src/integrations/undici/index.ts

@@ -89,7 +93,7 @@ export class Undici implements Integration {
      const url = new URL(request.path, request.origin);
      const stringUrl = url.toString();

-      if (isSentryRequest(stringUrl)) {
+      if (isSentryRequest(stringUrl) || request.__sentry__ !== undefined) {


Hmm this looks strange. As if the same request went twice through this channel? Anyway, I think it's fine to bail out in this case

Lms24 · 2023-03-28T07:16:25Z

packages/node/test/integrations/undici.test.ts

+  it('does not create a span for sentry requests', async () => {
+    const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;
+    hub.getScope().setSpan(transaction);
+
+    try {
+      await fetch(`${SENTRY_DSN}/sub/route`, {
+        method: 'POST',
+      });
+    } catch (e) {
+      // ignore
+    }
+
+    expect(transaction.spanRecorder?.spans.length).toBe(1);
+  });
+
+  it('does not create a span for sentry requests', async () => {
+    const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;
+    hub.getScope().setSpan(transaction);
+
+    expect(transaction.spanRecorder?.spans.length).toBe(1);
+
+    try {
+      await fetch(`${SENTRY_DSN}/sub/route`, {
+        method: 'POST',
+      });
+    } catch (e) {
+      // ignore
+    }
+
+    expect(transaction.spanRecorder?.spans.length).toBe(1);
+  });


l: duplicate?

Suggested change

it('does not create a span for sentry requests', async () => {

const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;

hub.getScope().setSpan(transaction);

try {

await fetch(`${SENTRY_DSN}/sub/route`, {

method: 'POST',

});

} catch (e) {

// ignore

}

expect(transaction.spanRecorder?.spans.length).toBe(1);

});

it('does not create a span for sentry requests', async () => {

const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;

hub.getScope().setSpan(transaction);

expect(transaction.spanRecorder?.spans.length).toBe(1);

try {

await fetch(`${SENTRY_DSN}/sub/route`, {

method: 'POST',

});

} catch (e) {

// ignore

}

expect(transaction.spanRecorder?.spans.length).toBe(1);

});

it('does not create a span for sentry requests', async () => {

const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;

hub.getScope().setSpan(transaction);

try {

await fetch(`${SENTRY_DSN}/sub/route`, {

method: 'POST',

});

} catch (e) {

// ignore

}

expect(transaction.spanRecorder?.spans.length).toBe(1);

});

Lms24 · 2023-03-28T07:25:38Z

packages/node/test/integrations/undici.test.ts

+      await fetch(`${SENTRY_DSN}/sub/route`, {
+        method: 'POST',
+      });


l (and more a question than a suggestion):
This makes me wonder about our isSentryRequest check. As I mentioned yesterday, we have 5 slightly different checks for identifying a sentry request all over the code base. I'll sooner or later clean this up (after #7626) but should this check take the Http method into account? Some checks do (example) while others (like the one you're using here) don't. I'd argue it's probably not necessary to check for the method but wdyt?

All sentry requests use POST atm, so I don't see anything wrong with this for now, but the more future-proof and correct solution is to not take into account the http method (in case we do ever introduce GET methods - think something like dynamic config).

Going to keep this as is for the test, we can come back and refactor all instances of isSentryRequest later.

Lms24 · 2023-03-28T07:29:34Z

packages/node/test/integrations/undici.test.ts

+  it('does create a span if `shouldCreateSpanForRequest` is defined', async () => {
+    const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;
+    hub.getScope().setSpan(transaction);
+
+    const client = new NodeClient({ ...DEFAULT_OPTIONS, shouldCreateSpanForRequest: url => url.includes('yes') });
+    hub.bindClient(client);
+
+    await fetch('http://localhost:18099/no', { method: 'POST' });
+
+    expect(transaction.spanRecorder?.spans.length).toBe(1);
+
+    await fetch('http://localhost:18099/yes', { method: 'POST' });
+
+    expect(transaction.spanRecorder?.spans.length).toBe(2);
+  });
+
+  it('attaches the sentry trace and baggage headers', async () => {
+    const transaction = hub.startTransaction({ name: 'test-transaction' }) as Transaction;
+    hub.getScope().setSpan(transaction);
+
+    await fetch('http://localhost:18099', { method: 'POST' });
+
+    expect(transaction.spanRecorder?.spans.length).toBe(2);
+    const span = transaction.spanRecorder?.spans[1];
+
+    expect(requestHeaders['sentry-trace']).toEqual(span?.toTraceparent());
+    expect(requestHeaders['baggage']).toEqual(
+      `sentry-environment=production,sentry-transaction=test-transaction,sentry-public_key=0,sentry-trace_id=${transaction.traceId},sentry-sample_rate=1`,
+    );
+  });


l: Let's add a test to cover the case where shouldCreateSpanForRequest returns false which should lead to the tracing headers not being attached.

github-actions · 2023-03-29T09:10:25Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.62 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	64.4 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.15 KB (-0.03% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	56.78 KB (+0.02% 🔺)
@sentry/browser - Webpack (gzipped + minified)	21.53 KB (-0.39% 🔽)
@sentry/browser - Webpack (minified)	72 KB (-0.04% 🔽)
@sentry/react - Webpack (gzipped + minified)	21.55 KB (-0.39% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	51.98 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.16 KB (+0.06% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.36 KB (+0.06% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	44.74 KB (+0.61% 🔺)
@sentry/replay - Webpack (gzipped + minified)	38.86 KB (+0.83% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	63.41 KB (+0.46% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	56.49 KB (+0.49% 🔺)

AbhiPrasad commented Mar 27, 2023

View reviewed changes

AbhiPrasad mentioned this pull request Mar 27, 2023

Undici integration final touches #7624

Closed

Lms24 approved these changes Mar 28, 2023

View reviewed changes

AbhiPrasad added 12 commits March 29, 2023 10:39

feat(node): Undici integration

c94fdaa

quite a bit more code

67c5dbd

this is gonna be a pain to test

aaa3dbf

update to latest dc

9d1ba89

re-organize

cd76d6a

remove .gitignore

8128d79

use dynamic require

fee6cc0

lint

956ef49

test(node): Add tests for Undici

15e9446

add undici dep

1834a71

conditionally require fetch

3931a20

update test based on PR review feedback

3dd7943

AbhiPrasad force-pushed the abhi-undici-test branch from 7d88183 to 3dd7943 Compare March 29, 2023 08:50

AbhiPrasad enabled auto-merge (squash) March 29, 2023 08:50

AbhiPrasad merged commit 32675e8 into develop Mar 29, 2023

AbhiPrasad deleted the abhi-undici-test branch March 29, 2023 09:29

AbhiPrasad self-assigned this Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(node): Add tests for Undici #7628

test(node): Add tests for Undici #7628

AbhiPrasad commented Mar 27, 2023

AbhiPrasad Mar 27, 2023

Lms24 Mar 28, 2023

Lms24 left a comment

Lms24 Mar 28, 2023

Lms24 Mar 28, 2023

Lms24 Mar 28, 2023

Lms24 Mar 28, 2023

AbhiPrasad Mar 29, 2023

Lms24 Mar 28, 2023

github-actions bot commented Mar 29, 2023

test(node): Add tests for Undici #7628

test(node): Add tests for Undici #7628

Conversation

AbhiPrasad commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 29, 2023

size-limit report 📦