Skip to content

Commit 1bffafa

Browse files
authored
fix(instrumentation-http): guard against double-instrumentation if loaded with 'require' and 'import' (#6437)
1 parent f7cd6ab commit 1bffafa

File tree

4 files changed

+239
-1
lines changed

4 files changed

+239
-1
lines changed

experimental/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2
1212

1313
### :bug: Bug Fixes
1414

15+
* fix(instrumentation-http): guard against double-instrumentation if loaded with `require('http')` and `import 'http'` [#6428](https://github.com/open-telemetry/opentelemetry-js/issues/6428) @trentm
1516
* fix(otlp-exporter-base): handle response error [#6412](https://github.com/open-telemetry/opentelemetry-js/pull/6412) @pichlermarc
1617
* Fixes a bug where when the response header was received, but the connection was reset by the server,
1718
an unhandled error would be thrown.

experimental/packages/opentelemetry-instrumentation-http/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@
1111
"clean": "tsc --build --clean",
1212
"test:cjs": "nyc mocha test/**/*.test.ts",
1313
"test:esm": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/**/*.test.mjs'",
14-
"test": "npm run test:cjs && npm run test:esm",
14+
"test:double-instr": "nyc node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/integrations/double-instr.test.cjs'",
15+
"test": "npm run test:cjs && npm run test:esm && npm run test:double-instr",
1516
"tdd": "npm run test -- --watch-extensions ts --watch",
1617
"lint": "eslint . --ext .ts",
1718
"lint:fix": "eslint . --ext .ts --fix",

experimental/packages/opentelemetry-instrumentation-http/src/http.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16+
1617
import {
1718
context,
1819
HrTime,
@@ -91,6 +92,8 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
9192
/** keep track on spans not ended */
9293
private readonly _spanNotEnded: WeakSet<Span> = new WeakSet<Span>();
9394
private _headerCapture;
95+
private _httpPatched: boolean = false;
96+
private _httpsPatched: boolean = false;
9497
declare private _oldHttpServerDurationHistogram: Histogram;
9598
declare private _stableHttpServerDurationHistogram: Histogram;
9699
declare private _oldHttpClientDurationHistogram: Histogram;
@@ -209,8 +212,16 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
209212
'http',
210213
['*'],
211214
(moduleExports: Http): Http => {
215+
// Guard against double-instrumentation, if loaded by both `require`
216+
// and `import`.
217+
if (this._httpPatched) {
218+
return moduleExports;
219+
}
220+
this._httpPatched = true;
221+
212222
// eslint-disable-next-line @typescript-eslint/no-explicit-any
213223
const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module';
224+
214225
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
215226
const patchedRequest = this._wrap(
216227
moduleExports,
@@ -241,6 +252,7 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
241252
return moduleExports;
242253
},
243254
(moduleExports: Http) => {
255+
this._httpPatched = false;
244256
if (moduleExports === undefined) return;
245257

246258
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
@@ -259,8 +271,16 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
259271
'https',
260272
['*'],
261273
(moduleExports: Https): Https => {
274+
// Guard against double-instrumentation, if loaded by both `require`
275+
// and `import`.
276+
if (this._httpsPatched) {
277+
return moduleExports;
278+
}
279+
this._httpsPatched = true;
280+
262281
// eslint-disable-next-line @typescript-eslint/no-explicit-any
263282
const isESM = (moduleExports as any)[Symbol.toStringTag] === 'Module';
283+
264284
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
265285
const patchedRequest = this._wrap(
266286
moduleExports,
@@ -291,6 +311,7 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
291311
return moduleExports;
292312
},
293313
(moduleExports: Https) => {
314+
this._httpsPatched = false;
294315
if (moduleExports === undefined) return;
295316

296317
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
// Test that http/https are not *double*-instrumented if http is loaded by
18+
// both `require` and `import`.
19+
//
20+
// This is a separate .cjs file to (a) more clearly use `require` and `import`
21+
// without .ts transpilation muddying the issue and (b) to separate this
22+
// file from being run with other `*.test.ts` test files in this package,
23+
// because this needs to be run in a separate process to avoid crosstalk.
24+
25+
const assert = require('assert');
26+
const path = require('path');
27+
const fs = require('fs');
28+
29+
const { SpanKind } = require('@opentelemetry/api');
30+
const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
31+
const {
32+
InMemorySpanExporter,
33+
SimpleSpanProcessor,
34+
} = require('@opentelemetry/sdk-trace-base');
35+
36+
const { HttpInstrumentation } = require('../../build/src/index.js');
37+
38+
const memoryExporter = new InMemorySpanExporter();
39+
const provider = new NodeTracerProvider({
40+
spanProcessors: [new SimpleSpanProcessor(memoryExporter)],
41+
});
42+
const instrumentation = new HttpInstrumentation();
43+
instrumentation.setTracerProvider(provider);
44+
45+
/**
46+
* Each of the test cases expects two spans: the http(s) SERVER span and the
47+
* single http(s) CLIENT span.
48+
*/
49+
function assertTwoSpans(spans, opts) {
50+
assert.strictEqual(spans.length, 2);
51+
for (const span of spans) {
52+
assert.strictEqual(
53+
span.instrumentationScope.name,
54+
'@opentelemetry/instrumentation-http'
55+
);
56+
if (opts.pathname) {
57+
assert.strictEqual(span.attributes['http.target'], opts.pathname);
58+
}
59+
}
60+
assert.strictEqual(spans[0].kind, SpanKind.SERVER);
61+
assert.strictEqual(spans[1].kind, SpanKind.CLIENT);
62+
}
63+
64+
describe('HttpInstrumentation double-instr tests', function () {
65+
let rhttp;
66+
let ihttp;
67+
let rhttps;
68+
let ihttps;
69+
70+
before(async () => {
71+
// arrange
72+
// This is the main thing being tested: does instr-http work when 'http'
73+
// has been loaded by both `require` and `import`.
74+
rhttp = require('http');
75+
ihttp = await import('http');
76+
rhttps = require('https');
77+
ihttps = await import('https');
78+
});
79+
80+
beforeEach(() => {
81+
memoryExporter.reset();
82+
});
83+
84+
describe('http', function () {
85+
let port;
86+
let server;
87+
88+
before(done => {
89+
server = rhttp.createServer((req, res) => {
90+
req.resume();
91+
req.on('end', () => {
92+
res.writeHead(200);
93+
res.end('pong');
94+
});
95+
});
96+
97+
server.listen(0, '127.0.0.1', () => {
98+
port = server.address().port;
99+
assert.ok(Number.isInteger(port));
100+
done();
101+
});
102+
});
103+
104+
after(done => {
105+
server.close(done);
106+
});
107+
108+
it('one span for one http.request (require)', async function () {
109+
// act
110+
await new Promise(resolve => {
111+
const clientReq = rhttp.request(
112+
`http://127.0.0.1:${port}/rhttp.request`,
113+
clientRes => {
114+
clientRes.resume();
115+
clientRes.on('end', resolve);
116+
}
117+
);
118+
clientReq.end();
119+
});
120+
121+
// assert
122+
const spans = memoryExporter.getFinishedSpans();
123+
assertTwoSpans(spans, { pathname: '/rhttp.request' });
124+
});
125+
126+
it('one span for one http.request (import)', async function () {
127+
await new Promise(resolve => {
128+
const clientReq = ihttp.request(
129+
`http://127.0.0.1:${port}/ihttp.request`,
130+
clientRes => {
131+
clientRes.resume();
132+
clientRes.on('end', resolve);
133+
}
134+
);
135+
clientReq.end();
136+
});
137+
138+
const spans = memoryExporter.getFinishedSpans();
139+
assertTwoSpans(spans, { pathname: '/ihttp.request' });
140+
});
141+
});
142+
143+
describe('https', function () {
144+
let port;
145+
let server;
146+
147+
before(done => {
148+
server = rhttps.createServer(
149+
{
150+
key: fs.readFileSync(
151+
path.resolve(__dirname, '../fixtures/server-key.pem')
152+
),
153+
cert: fs.readFileSync(
154+
path.resolve(__dirname, '../fixtures/server-cert.pem')
155+
),
156+
},
157+
(req, res) => {
158+
req.resume();
159+
req.on('end', () => {
160+
res.writeHead(200);
161+
res.end('pong');
162+
});
163+
}
164+
);
165+
166+
server.listen(0, '127.0.0.1', () => {
167+
port = server.address().port;
168+
assert.ok(Number.isInteger(port));
169+
done();
170+
});
171+
});
172+
173+
after(done => {
174+
server.close(done);
175+
});
176+
177+
it('one span for one https.request (require)', async function () {
178+
await new Promise(resolve => {
179+
const clientReq = rhttps.request(
180+
`https://127.0.0.1:${port}/rhttps.request`,
181+
{
182+
rejectUnauthorized: false,
183+
},
184+
clientRes => {
185+
clientRes.resume();
186+
clientRes.on('end', resolve);
187+
}
188+
);
189+
clientReq.end();
190+
});
191+
192+
const spans = memoryExporter.getFinishedSpans();
193+
assertTwoSpans(spans, { pathname: '/rhttps.request' });
194+
});
195+
196+
it('one span for one https.request (import)', async function () {
197+
await new Promise(resolve => {
198+
const clientReq = ihttps.request(
199+
`https://127.0.0.1:${port}/ihttps.request`,
200+
{
201+
rejectUnauthorized: false,
202+
},
203+
clientRes => {
204+
clientRes.resume();
205+
clientRes.on('end', resolve);
206+
}
207+
);
208+
clientReq.end();
209+
});
210+
211+
const spans = memoryExporter.getFinishedSpans();
212+
assertTwoSpans(spans, { pathname: '/ihttps.request' });
213+
});
214+
});
215+
});

0 commit comments

Comments
 (0)