Skip to content

Commit 7b052e3

Browse files
mydeamydea
committed
fixes
1 parent b432b1d commit 7b052e3

File tree

4 files changed

+301
-22
lines changed

4 files changed

+301
-22
lines changed

‎dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-overwrite-server-emit.mjs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import * as Sentry from '@sentry/node';
2+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
3+
4+
Sentry.init({
5+
dsn: 'https://[email protected]/1337',
6+
release: '1.0',
7+
tracesSampleRate: 1.0,
8+
transport: loggingTransport,
9+
// We deliberately disable dedupe here, to make sure we do not double wrap the emit function
10+
// Otherwise, duplicate spans (that we want to test against) may be dropped by dedupe detection
11+
integrations: integrations => integrations.filter(integration => integration.name !== 'Dedupe'),
12+
});

‎dev-packages/node-integration-tests/suites/tracing/httpIntegration/scenario-overwrite-server-emit.mjs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { sendPortToRunner } from '@sentry-internal/node-integration-tests';
2+
import http from 'http';
3+
4+
const server = http.createServer(function (request, response) {
5+
const useProxy = request.url.includes('proxy');
6+
const patchOriginalEmit = request.url.includes('original');
7+
8+
// Monkey patch it again to ensure it still works later
9+
// We cover multiple possible scenarios here:
10+
// 1. Use proxy to overwrite server.emit
11+
// 2. Use proxy to overwrite server.emit, using initial server.emit
12+
// 3. Use classic monkey patching to overwrite server.emit
13+
// 4. Use classic monkey patching to overwrite server.emit, using initial server.emit
14+
monkeyPatchEmit(server, { useProxy, patchOriginalEmit });
15+
16+
response.end('Hello Node.js Server!');
17+
});
18+
19+
const initialServerEmit = server.emit;
20+
21+
server.listen(0, () => {
22+
sendPortToRunner(server.address().port);
23+
});
24+
25+
function monkeyPatchEmit(server, { useProxy = false, patchOriginalEmit = false }) {
26+
const originalEmit = patchOriginalEmit ? initialServerEmit : server.emit;
27+
28+
if (useProxy) {
29+
server.emit = new Proxy(originalEmit, {
30+
apply(target, thisArg, args) {
31+
return target.apply(thisArg, args);
32+
},
33+
});
34+
} else {
35+
server.emit = function () {
36+
return originalEmit.apply(server, arguments);
37+
};
38+
}
39+
}

‎dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,246 @@ describe('httpIntegration', () => {
168168
await runner.completed();
169169
});
170170
});
171+
172+
describe('custom server.emit', () => {
173+
createEsmAndCjsTests(
174+
__dirname,
175+
'scenario-overwrite-server-emit.mjs',
176+
'instrument-overwrite-server-emit.mjs',
177+
(createRunner, test) => {
178+
test('handles server.emit being overwritten via classic monkey patching', async () => {
179+
const runner = createRunner()
180+
.expect({
181+
transaction: {
182+
transaction: 'GET /test1',
183+
contexts: {
184+
trace: {
185+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
186+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
187+
data: {
188+
'http.response.status_code': 200,
189+
'sentry.op': 'http.server',
190+
},
191+
},
192+
},
193+
spans: [],
194+
},
195+
})
196+
.expect({
197+
transaction: {
198+
transaction: 'GET /test2',
199+
contexts: {
200+
trace: {
201+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
202+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
203+
data: {
204+
'http.response.status_code': 200,
205+
'sentry.op': 'http.server',
206+
},
207+
},
208+
},
209+
spans: [],
210+
},
211+
})
212+
.expect({
213+
transaction: {
214+
transaction: 'GET /test3',
215+
contexts: {
216+
trace: {
217+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
218+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
219+
data: {
220+
'http.response.status_code': 200,
221+
'sentry.op': 'http.server',
222+
},
223+
},
224+
},
225+
spans: [],
226+
},
227+
})
228+
.start();
229+
230+
await runner.makeRequest('get', '/test1');
231+
await runner.makeRequest('get', '/test2');
232+
await runner.makeRequest('get', '/test3');
233+
await runner.completed();
234+
});
235+
236+
test('handles server.emit being overwritten via proxy', async () => {
237+
const runner = createRunner()
238+
.expect({
239+
transaction: {
240+
transaction: 'GET /test1-proxy',
241+
contexts: {
242+
trace: {
243+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
244+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
245+
data: {
246+
'http.response.status_code': 200,
247+
'sentry.op': 'http.server',
248+
},
249+
},
250+
},
251+
spans: [],
252+
},
253+
})
254+
.expect({
255+
transaction: {
256+
transaction: 'GET /test2-proxy',
257+
contexts: {
258+
trace: {
259+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
260+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
261+
data: {
262+
'http.response.status_code': 200,
263+
'sentry.op': 'http.server',
264+
},
265+
},
266+
},
267+
spans: [],
268+
},
269+
})
270+
.expect({
271+
transaction: {
272+
transaction: 'GET /test3-proxy',
273+
contexts: {
274+
trace: {
275+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
276+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
277+
data: {
278+
'http.response.status_code': 200,
279+
'sentry.op': 'http.server',
280+
},
281+
},
282+
},
283+
},
284+
})
285+
.start();
286+
287+
await runner.makeRequest('get', '/test1-proxy');
288+
await runner.makeRequest('get', '/test2-proxy');
289+
await runner.makeRequest('get', '/test3-proxy');
290+
await runner.completed();
291+
});
292+
293+
test('handles server.emit being overwritten via classic monkey patching, using initial server.emit', async () => {
294+
const runner = createRunner()
295+
.expect({
296+
transaction: {
297+
transaction: 'GET /test1-original',
298+
contexts: {
299+
trace: {
300+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
301+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
302+
data: {
303+
'http.response.status_code': 200,
304+
'sentry.op': 'http.server',
305+
},
306+
},
307+
},
308+
spans: [],
309+
},
310+
})
311+
.expect({
312+
transaction: {
313+
transaction: 'GET /test2-original',
314+
contexts: {
315+
trace: {
316+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
317+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
318+
data: {
319+
'http.response.status_code': 200,
320+
'sentry.op': 'http.server',
321+
},
322+
},
323+
},
324+
spans: [],
325+
},
326+
})
327+
.expect({
328+
transaction: {
329+
transaction: 'GET /test3-original',
330+
contexts: {
331+
trace: {
332+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
333+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
334+
data: {
335+
'http.response.status_code': 200,
336+
'sentry.op': 'http.server',
337+
},
338+
},
339+
},
340+
spans: [],
341+
},
342+
})
343+
.start();
344+
345+
await runner.makeRequest('get', '/test1-original');
346+
await runner.makeRequest('get', '/test2-original');
347+
await runner.makeRequest('get', '/test3-original');
348+
await runner.completed();
349+
});
350+
351+
test('handles server.emit being overwritten via proxy, using initial server.emit', async () => {
352+
const runner = createRunner()
353+
.expect({
354+
transaction: {
355+
transaction: 'GET /test1-proxy-original',
356+
contexts: {
357+
trace: {
358+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
359+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
360+
data: {
361+
'http.response.status_code': 200,
362+
'sentry.op': 'http.server',
363+
},
364+
},
365+
},
366+
spans: [],
367+
},
368+
})
369+
.expect({
370+
transaction: {
371+
transaction: 'GET /test2-proxy-original',
372+
contexts: {
373+
trace: {
374+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
375+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
376+
data: {
377+
'http.response.status_code': 200,
378+
'sentry.op': 'http.server',
379+
},
380+
},
381+
},
382+
spans: [],
383+
},
384+
})
385+
.expect({
386+
transaction: {
387+
transaction: 'GET /test3-proxy-original',
388+
contexts: {
389+
trace: {
390+
span_id: expect.stringMatching(/[a-f0-9]{16}/),
391+
trace_id: expect.stringMatching(/[a-f0-9]{32}/),
392+
data: {
393+
'http.response.status_code': 200,
394+
'sentry.op': 'http.server',
395+
},
396+
},
397+
},
398+
spans: [],
399+
},
400+
})
401+
.start();
402+
403+
await runner.makeRequest('get', '/test1-proxy-original');
404+
await runner.makeRequest('get', '/test2-proxy-original');
405+
await runner.makeRequest('get', '/test3-proxy-original');
406+
await runner.completed();
407+
});
408+
},
409+
);
410+
});
171411
});
172412

173413
describe("doesn't create a root span for incoming requests ignored via `ignoreIncomingRequests`", () => {

‎packages/node-core/src/integrations/http/incoming-requests.ts

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* eslint-disable max-lines */
22
import type { Span } from '@opentelemetry/api';
3-
import { context, createContextKey , propagation, SpanKind, trace } from '@opentelemetry/api';
3+
import { context, createContextKey, propagation, SpanKind, trace } from '@opentelemetry/api';
44
import type { RPCMetadata } from '@opentelemetry/core';
55
import { getRPCMetadata, isTracingSuppressed, RPCType, setRPCMetadata } from '@opentelemetry/core';
66
import {
@@ -13,7 +13,6 @@ import {
1313
} from '@opentelemetry/semantic-conventions';
1414
import type { AggregationCounts, Client, Scope, SpanAttributes } from '@sentry/core';
1515
import {
16-
addNonEnumerableProperty,
1716
debug,
1817
generateSpanId,
1918
getClient,
@@ -32,7 +31,6 @@ import type EventEmitter from 'events';
3231
import { errorMonitor } from 'events';
3332
import type { ClientRequest, IncomingHttpHeaders, IncomingMessage, Server, ServerResponse } from 'http';
3433
import type { Socket } from 'net';
35-
import { isProxy } from 'util/types';
3634
import { DEBUG_BUILD } from '../../debug-build';
3735
import type { NodeClient } from '../../sdk/client';
3836
import { INSTRUMENTATION_NAME, MAX_BODY_BYTE_LENGTH } from './constants';
@@ -49,6 +47,11 @@ const clientToRequestSessionAggregatesMap = new Map<
4947
{ [timestampRoundedToSeconds: string]: { exited: number; crashed: number; errored: number } }
5048
>();
5149

50+
// We keep track of emit functions we wrapped, to avoid double wrapping
51+
// We do this instead of putting a non-enumerable property on the function, because
52+
// sometimes the property seems to be migrated to forks of the emit function, which we do not want to happen
53+
const wrappedEmitFns = new WeakSet<Emit>();
54+
5255
/**
5356
* Instrument a server to capture incoming requests.
5457
*
@@ -92,7 +95,7 @@ export function instrumentServer(
9295
// eslint-disable-next-line @typescript-eslint/unbound-method
9396
const originalEmit: Emit = server.emit;
9497

95-
if (isEmitInstrumented(originalEmit)) {
98+
if (wrappedEmitFns.has(originalEmit)) {
9699
DEBUG_BUILD &&
97100
debug.log(INSTRUMENTATION_NAME, 'Incoming requests already instrumented, not instrumenting again...');
98101
return;
@@ -134,7 +137,8 @@ export function instrumentServer(
134137
return target.apply(thisArg, args);
135138
}
136139

137-
// Make sure we do not double wrap this, for edge cases...
140+
// Make sure we do not double execute our wrapper code, for edge cases...
141+
// Without this check, if we double-wrap emit, for whatever reason, you'd get to http.server spans (one the children of the other)
138142
if (context.active().getValue(HTTP_SERVER_INSTRUMENTED_KEY)) {
139143
return target.apply(thisArg, args);
140144
}
@@ -281,7 +285,7 @@ export function instrumentServer(
281285
},
282286
});
283287

284-
addNonEnumerableProperty(newEmit, '__sentry_patched__', true);
288+
wrappedEmitFns.add(newEmit);
285289
server.emit = newEmit;
286290
}
287291

@@ -570,19 +574,3 @@ export function isStaticAssetRequest(urlPath: string): boolean {
570574

571575
return false;
572576
}
573-
574-
function isEmitInstrumented(emit: Emit): boolean {
575-
// Easy: it does not have a __sentry_patched__ property? def. not instrumented
576-
if (!('__sentry_patched__' in emit)) {
577-
return false;
578-
}
579-
580-
// In weird cases with eventemitter2, it can _still_ be un-patched even if this propery is set :sad:
581-
// In this case, emit is not a proxy, which means it is not what we set it to
582-
// We'll wrap emit again in this case - but we also have code to ensure we do not double run our code inside of the wrapped emit
583-
if (!isProxy(emit)) {
584-
return false;
585-
}
586-
587-
return true;
588-
}

0 commit comments

Comments
 (0)