Skip to content

Commit 6e2eaed

Browse files
mydeamydea
authored
feat(react-router): Ensure http.server route handling is consistent (#16986)
This updates react router http.server handling to ensure: * That we always have a correct route/url source * Streamlines how we get rid of the weird `*` transaction name by just using `processEvent` in the integration. This means we now always add the server integration, it just does less in non-matching node versions. * Ensure we always have a good span origin * Ensure the name of the spans is consistent with the target (e.g. containing the .data suffix) - we can revisit this later but for now this is consistent I'd say.
1 parent 90c2385 commit 6e2eaed

File tree

18 files changed

+351
-195
lines changed

18 files changed

+351
-195
lines changed

‎dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/instrument.mjs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,4 @@ Sentry.init({
55
environment: 'qa', // dynamic sampling bias to keep transactions
66
tracesSampleRate: 1.0,
77
tunnel: `http://localhost:3031/`, // proxy server
8-
integrations: function (integrations) {
9-
return integrations.filter(integration => {
10-
return integration.name !== 'ReactRouterServer';
11-
});
12-
},
138
});

‎dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/performance/performance.server.test.ts‎

Lines changed: 79 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { expect, test } from '@playwright/test';
22
import { waitForTransaction } from '@sentry-internal/test-utils';
33
import { APP_NAME } from '../constants';
44

5-
test.describe('servery - performance', () => {
5+
test.describe('server - performance', () => {
66
test('should send server transaction on pageload', async ({ page }) => {
77
const txPromise = waitForTransaction(APP_NAME, async transactionEvent => {
88
return transactionEvent.transaction === 'GET /performance';
@@ -19,11 +19,11 @@ test.describe('servery - performance', () => {
1919
trace_id: expect.any(String),
2020
data: {
2121
'sentry.op': 'http.server',
22-
'sentry.origin': 'auto.http.otel.http',
22+
'sentry.origin': 'auto.http.react-router.request-handler',
2323
'sentry.source': 'route',
2424
},
2525
op: 'http.server',
26-
origin: 'auto.http.otel.http',
26+
origin: 'auto.http.react-router.request-handler',
2727
},
2828
},
2929
spans: expect.any(Array),
@@ -70,11 +70,11 @@ test.describe('servery - performance', () => {
7070
trace_id: expect.any(String),
7171
data: {
7272
'sentry.op': 'http.server',
73-
'sentry.origin': 'auto.http.otel.http',
73+
'sentry.origin': 'auto.http.react-router.request-handler',
7474
'sentry.source': 'route',
7575
},
7676
op: 'http.server',
77-
origin: 'auto.http.otel.http',
77+
origin: 'auto.http.react-router.request-handler',
7878
},
7979
},
8080
spans: expect.any(Array),
@@ -107,21 +107,52 @@ test.describe('servery - performance', () => {
107107

108108
test('should instrument wrapped server loader', async ({ page }) => {
109109
const txPromise = waitForTransaction(APP_NAME, async transactionEvent => {
110-
console.log(110, transactionEvent.transaction);
111-
return transactionEvent.transaction === 'GET /performance/server-loader';
110+
return transactionEvent.transaction === 'GET /performance/server-loader.data';
112111
});
113112

114113
await page.goto(`/performance`);
115-
await page.waitForTimeout(500);
116114
await page.getByRole('link', { name: 'Server Loader' }).click();
117115

118116
const transaction = await txPromise;
119117

120-
expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({
118+
expect(transaction).toEqual(
119+
expect.objectContaining({
120+
contexts: expect.objectContaining({
121+
trace: {
122+
span_id: expect.any(String),
123+
trace_id: expect.any(String),
124+
op: 'http.server',
125+
origin: 'auto.http.react-router.loader',
126+
parent_span_id: expect.any(String),
127+
status: 'ok',
128+
data: expect.objectContaining({
129+
'http.method': 'GET',
130+
'http.response.status_code': 200,
131+
'http.status_code': 200,
132+
'http.status_text': 'OK',
133+
'http.target': '/performance/server-loader.data',
134+
'http.url': 'http://localhost:3030/performance/server-loader.data',
135+
'sentry.op': 'http.server',
136+
'sentry.origin': 'auto.http.react-router.loader',
137+
'sentry.source': 'url',
138+
url: 'http://localhost:3030/performance/server-loader.data',
139+
}),
140+
},
141+
}),
142+
transaction: 'GET /performance/server-loader.data',
143+
type: 'transaction',
144+
transaction_info: { source: 'url' },
145+
platform: 'node',
146+
}),
147+
);
148+
// ensure we do not have a stray, bogus route attribute
149+
expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined();
150+
151+
expect(transaction?.spans).toContainEqual({
121152
span_id: expect.any(String),
122153
trace_id: expect.any(String),
123154
data: {
124-
'sentry.origin': 'auto.http.react-router',
155+
'sentry.origin': 'auto.http.react-router.loader',
125156
'sentry.op': 'function.react-router.loader',
126157
},
127158
description: 'Executing Server Loader',
@@ -130,25 +161,58 @@ test.describe('servery - performance', () => {
130161
timestamp: expect.any(Number),
131162
status: 'ok',
132163
op: 'function.react-router.loader',
133-
origin: 'auto.http.react-router',
164+
origin: 'auto.http.react-router.loader',
134165
});
135166
});
136167

137168
test('should instrument a wrapped server action', async ({ page }) => {
138169
const txPromise = waitForTransaction(APP_NAME, async transactionEvent => {
139-
return transactionEvent.transaction === 'POST /performance/server-action';
170+
return transactionEvent.transaction === 'POST /performance/server-action.data';
140171
});
141172

142173
await page.goto(`/performance/server-action`);
143174
await page.getByRole('button', { name: 'Submit' }).click();
144175

145176
const transaction = await txPromise;
146177

147-
expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({
178+
expect(transaction).toEqual(
179+
expect.objectContaining({
180+
contexts: expect.objectContaining({
181+
trace: {
182+
span_id: expect.any(String),
183+
trace_id: expect.any(String),
184+
op: 'http.server',
185+
origin: 'auto.http.react-router.action',
186+
parent_span_id: expect.any(String),
187+
status: 'ok',
188+
data: expect.objectContaining({
189+
'http.method': 'POST',
190+
'http.response.status_code': 200,
191+
'http.status_code': 200,
192+
'http.status_text': 'OK',
193+
'http.target': '/performance/server-action.data',
194+
'http.url': 'http://localhost:3030/performance/server-action.data',
195+
'sentry.op': 'http.server',
196+
'sentry.origin': 'auto.http.react-router.action',
197+
'sentry.source': 'url',
198+
url: 'http://localhost:3030/performance/server-action.data',
199+
}),
200+
},
201+
}),
202+
transaction: 'POST /performance/server-action.data',
203+
type: 'transaction',
204+
transaction_info: { source: 'url' },
205+
platform: 'node',
206+
}),
207+
);
208+
// ensure we do not have a stray, bogus route attribute
209+
expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined();
210+
211+
expect(transaction?.spans).toContainEqual({
148212
span_id: expect.any(String),
149213
trace_id: expect.any(String),
150214
data: {
151-
'sentry.origin': 'auto.http.react-router',
215+
'sentry.origin': 'auto.http.react-router.action',
152216
'sentry.op': 'function.react-router.action',
153217
},
154218
description: 'Executing Server Action',
@@ -157,7 +221,7 @@ test.describe('servery - performance', () => {
157221
timestamp: expect.any(Number),
158222
status: 'ok',
159223
op: 'function.react-router.action',
160-
origin: 'auto.http.react-router',
224+
origin: 'auto.http.react-router.action',
161225
});
162226
});
163227
});

‎dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/instrument.mjs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
import * as Sentry from '@sentry/react-router';
22

33
Sentry.init({
4-
// todo: grab from env
54
dsn: 'https://username@domain/123',
65
environment: 'qa', // dynamic sampling bias to keep transactions
76
tracesSampleRate: 1.0,
8-
tunnel: `http://localhost:3031/`, // proxy server
7+
tunnel: `http://localhost:3031/`, // proxy server,
98
});

‎dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/performance/performance.server.test.ts‎

Lines changed: 83 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { expect, test } from '@playwright/test';
22
import { waitForTransaction } from '@sentry-internal/test-utils';
33
import { APP_NAME } from '../constants';
44

5-
test.describe('servery - performance', () => {
5+
test.describe('server - performance', () => {
66
test('should send server transaction on pageload', async ({ page }) => {
77
const txPromise = waitForTransaction(APP_NAME, async transactionEvent => {
88
return transactionEvent.transaction === 'GET /performance';
@@ -19,11 +19,11 @@ test.describe('servery - performance', () => {
1919
trace_id: expect.any(String),
2020
data: {
2121
'sentry.op': 'http.server',
22-
'sentry.origin': 'auto.http.otel.http',
22+
'sentry.origin': 'auto.http.react-router.request-handler',
2323
'sentry.source': 'route',
2424
},
2525
op: 'http.server',
26-
origin: 'auto.http.otel.http',
26+
origin: 'auto.http.react-router.request-handler',
2727
},
2828
},
2929
spans: expect.any(Array),
@@ -70,11 +70,11 @@ test.describe('servery - performance', () => {
7070
trace_id: expect.any(String),
7171
data: {
7272
'sentry.op': 'http.server',
73-
'sentry.origin': 'auto.http.otel.http',
73+
'sentry.origin': 'auto.http.react-router.request-handler',
7474
'sentry.source': 'route',
7575
},
7676
op: 'http.server',
77-
origin: 'auto.http.otel.http',
77+
origin: 'auto.http.react-router.request-handler',
7878
},
7979
},
8080
spans: expect.any(Array),
@@ -110,26 +110,59 @@ test.describe('servery - performance', () => {
110110
return transactionEvent.transaction === 'GET /performance/server-loader.data';
111111
});
112112

113-
await page.goto(`/performance`); // initial ssr pageloads do not contain .data requests
114-
await page.waitForTimeout(500); // quick breather before navigation
113+
await page.goto('/performance'); // initial ssr pageloads do not contain .data requests
115114
await page.getByRole('link', { name: 'Server Loader' }).click(); // this will actually trigger a .data request
116115

117116
const transaction = await txPromise;
118117

119-
expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({
118+
expect(transaction).toEqual(
119+
expect.objectContaining({
120+
contexts: expect.objectContaining({
121+
trace: {
122+
span_id: expect.any(String),
123+
trace_id: expect.any(String),
124+
op: 'http.server',
125+
origin: 'auto.http.react-router.server',
126+
parent_span_id: expect.any(String),
127+
status: 'ok',
128+
data: expect.objectContaining({
129+
'http.method': 'GET',
130+
'http.response.status_code': 200,
131+
'http.status_code': 200,
132+
'http.status_text': 'OK',
133+
'http.target': '/performance/server-loader.data',
134+
'http.url': 'http://localhost:3030/performance/server-loader.data',
135+
'sentry.op': 'http.server',
136+
'sentry.origin': 'auto.http.react-router.server',
137+
'sentry.source': 'url',
138+
url: 'http://localhost:3030/performance/server-loader.data',
139+
}),
140+
},
141+
}),
142+
transaction: 'GET /performance/server-loader.data',
143+
type: 'transaction',
144+
transaction_info: { source: 'url' },
145+
platform: 'node',
146+
}),
147+
);
148+
149+
// ensure we do not have a stray, bogus route attribute
150+
expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined();
151+
152+
expect(transaction.spans).toContainEqual({
120153
span_id: expect.any(String),
121154
trace_id: expect.any(String),
122155
data: {
123-
'sentry.origin': 'auto.http.react-router',
124156
'sentry.op': 'function.react-router.loader',
157+
'sentry.origin': 'auto.http.react-router.server',
125158
},
126159
description: 'Executing Server Loader',
160+
op: 'function.react-router.loader',
161+
origin: 'auto.http.react-router.server',
127162
parent_span_id: expect.any(String),
128163
start_timestamp: expect.any(Number),
129-
timestamp: expect.any(Number),
130164
status: 'ok',
131-
op: 'function.react-router.loader',
132-
origin: 'auto.http.react-router',
165+
timestamp: expect.any(Number),
133166
});
134167
});
135168

@@ -143,20 +176,53 @@ test.describe('servery - performance', () => {
143176

144177
const transaction = await txPromise;
145178

146-
expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({
179+
expect(transaction).toEqual(
180+
expect.objectContaining({
181+
contexts: expect.objectContaining({
182+
trace: {
183+
span_id: expect.any(String),
184+
trace_id: expect.any(String),
185+
op: 'http.server',
186+
origin: 'auto.http.react-router.server',
187+
parent_span_id: expect.any(String),
188+
status: 'ok',
189+
data: expect.objectContaining({
190+
'http.method': 'POST',
191+
'http.response.status_code': 200,
192+
'http.status_code': 200,
193+
'http.status_text': 'OK',
194+
'http.target': '/performance/server-action.data',
195+
'http.url': 'http://localhost:3030/performance/server-action.data',
196+
'sentry.op': 'http.server',
197+
'sentry.origin': 'auto.http.react-router.server',
198+
'sentry.source': 'url',
199+
url: 'http://localhost:3030/performance/server-action.data',
200+
}),
201+
},
202+
}),
203+
transaction: 'POST /performance/server-action.data',
204+
type: 'transaction',
205+
transaction_info: { source: 'url' },
206+
platform: 'node',
207+
}),
208+
);
209+
// ensure we do not have a stray, bogus route attribute
210+
expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined();
211+
212+
expect(transaction.spans).toContainEqual({
147213
span_id: expect.any(String),
148214
trace_id: expect.any(String),
149215
data: {
150-
'sentry.origin': 'auto.http.react-router',
151216
'sentry.op': 'function.react-router.action',
217+
'sentry.origin': 'auto.http.react-router.server',
152218
},
153219
description: 'Executing Server Action',
220+
op: 'function.react-router.action',
221+
origin: 'auto.http.react-router.server',
154222
parent_span_id: expect.any(String),
155223
start_timestamp: expect.any(Number),
156-
timestamp: expect.any(Number),
157224
status: 'ok',
158-
op: 'function.react-router.action',
159-
origin: 'auto.http.react-router',
225+
timestamp: expect.any(Number),
160226
});
161227
});
162228
});

‎dev-packages/e2e-tests/test-applications/react-router-7-framework/instrument.mjs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import * as Sentry from '@sentry/react-router';
22

33
Sentry.init({
4-
// todo: grab from env
54
dsn: 'https://username@domain/123',
65
environment: 'qa', // dynamic sampling bias to keep transactions
76
tracesSampleRate: 1.0,

‎dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/performance/performance.server.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@ test.describe('server - performance', () => {
1919
trace_id: expect.any(String),
2020
data: {
2121
'sentry.op': 'http.server',
22-
'sentry.origin': 'auto.http.otel.http',
22+
'sentry.origin': 'auto.http.react-router.request-handler',
2323
'sentry.source': 'route',
2424
},
2525
op: 'http.server',
26-
origin: 'auto.http.otel.http',
26+
origin: 'auto.http.react-router.request-handler',
2727
},
2828
},
2929
spans: expect.any(Array),
@@ -70,11 +70,11 @@ test.describe('server - performance', () => {
7070
trace_id: expect.any(String),
7171
data: {
7272
'sentry.op': 'http.server',
73-
'sentry.origin': 'auto.http.otel.http',
73+
'sentry.origin': 'auto.http.react-router.request-handler',
7474
'sentry.source': 'route',
7575
},
7676
op: 'http.server',
77-
origin: 'auto.http.otel.http',
77+
origin: 'auto.http.react-router.request-handler',
7878
},
7979
},
8080
spans: expect.any(Array),

0 commit comments

Comments
 (0)