Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 7b052e3

Browse files
committed
fixes
1 parent b432b1d commit 7b052e3

File tree

4 files changed

+301
-22
lines changed

4 files changed

+301
-22
lines changed
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://public@dsn.ingest.sentry.io/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+
});
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)

AltStyle によって変換されたページ (->オリジナル) /