Skip to content

Commit ca70bb9

Browse files
david-lunatrentm
david-luna
and
trentm
authored
fix(instr-undici): fix issue with config in constructor (#2395)
Co-authored-by: Trent Mick <[email protected]>
1 parent 7054bc1 commit ca70bb9

File tree

4 files changed

+76
-91
lines changed

4 files changed

+76
-91
lines changed

‎plugins/node/instrumentation-undici/src/undici.ts‎

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
6868
private _httpClientDurationHistogram!: Histogram;
6969
constructor(config: UndiciInstrumentationConfig = {}) {
7070
super(PACKAGE_NAME, PACKAGE_VERSION, config);
71-
this.setConfig(config);
7271
}
7372

7473
// No need to instrument files/modules
@@ -77,26 +76,32 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
7776
}
7877

7978
override disable(): void {
80-
if (!this.getConfig().enabled) {
81-
return;
82-
}
83-
79+
super.disable();
8480
this._channelSubs.forEach(sub => sub.channel.unsubscribe(sub.onMessage));
8581
this._channelSubs.length = 0;
86-
super.disable();
87-
this.setConfig({ ...this.getConfig(), enabled: false });
8882
}
8983

9084
override enable(): void {
91-
if (this.getConfig().enabled) {
92-
return;
93-
}
85+
// "enabled" handling is currently a bit messy with InstrumentationBase.
86+
// If constructed with `{enabled: false}`, this `.enable()` is still called,
87+
// and `this.getConfig().enabled !== this.isEnabled()`, creating confusion.
88+
//
89+
// For now, this class will setup for instrumenting if `.enable()` is
90+
// called, but use `this.getConfig().enabled` to determine if
91+
// instrumentation should be generated. This covers the more likely common
92+
// case of config being given a construction time, rather than later via
93+
// `instance.enable()`, `.disable()`, or `.setConfig()` calls.
9494
super.enable();
95-
this.setConfig({ ...this.getConfig(), enabled: true });
9695

97-
// This method is called by the `InstrumentationAbstract` constructor before
98-
// ours is called. So we need to ensure the property is initalized
96+
// This method is called by the super-class constructor before ours is
97+
// called. So we need to ensure the property is initalized.
9998
this._channelSubs = this._channelSubs || [];
99+
100+
// Avoid to duplicate subscriptions
101+
if (this._channelSubs.length > 0) {
102+
return;
103+
}
104+
100105
this.subscribeToChannel(
101106
'undici:request:create',
102107
this.onRequestCreated.bind(this)
@@ -113,16 +118,6 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
113118
this.subscribeToChannel('undici:request:error', this.onError.bind(this));
114119
}
115120

116-
override setConfig(config: UndiciInstrumentationConfig = {}): void {
117-
super.setConfig(config);
118-
119-
if (config?.enabled) {
120-
this.enable();
121-
} else {
122-
this.disable();
123-
}
124-
}
125-
126121
protected override _updateMetricInstruments() {
127122
this._httpClientDurationHistogram = this.meter.createHistogram(
128123
'http.client.request.duration',
@@ -162,9 +157,10 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
162157
// - ignored by config
163158
// - method is 'CONNECT'
164159
const config = this.getConfig();
160+
const enabled = config.enabled !== false;
165161
const shouldIgnoreReq = safeExecuteInTheMiddle(
166162
() =>
167-
!config.enabled ||
163+
!enabled ||
168164
request.method === 'CONNECT' ||
169165
config.ignoreRequestHook?.(request),
170166
e => e && this._diag.error('caught ignoreRequestHook error: ', e),

‎plugins/node/instrumentation-undici/test/fetch.test.ts‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,16 @@ import { MockPropagation } from './utils/mock-propagation';
3535
import { MockServer } from './utils/mock-server';
3636
import { assertSpan } from './utils/assertSpan';
3737

38-
const instrumentation = new UndiciInstrumentation();
39-
instrumentation.enable();
40-
instrumentation.disable();
41-
42-
const protocol = 'http';
43-
const hostname = 'localhost';
44-
const mockServer = new MockServer();
45-
const memoryExporter = new InMemorySpanExporter();
46-
const provider = new NodeTracerProvider();
47-
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));
48-
instrumentation.setTracerProvider(provider);
49-
5038
describe('UndiciInstrumentation `fetch` tests', function () {
39+
let instrumentation: UndiciInstrumentation;
40+
41+
const protocol = 'http';
42+
const hostname = 'localhost';
43+
const mockServer = new MockServer();
44+
const memoryExporter = new InMemorySpanExporter();
45+
const provider = new NodeTracerProvider();
46+
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));
47+
5148
before(function (done) {
5249
// Do not test if the `fetch` global API is not available
5350
// This applies to nodejs < v18 or nodejs < v16.15 wihtout the flag
@@ -57,6 +54,9 @@ describe('UndiciInstrumentation `fetch` tests', function () {
5754
this.skip();
5855
}
5956

57+
instrumentation = new UndiciInstrumentation();
58+
instrumentation.setTracerProvider(provider);
59+
6060
propagation.setGlobalPropagator(new MockPropagation());
6161
context.setGlobalContextManager(new AsyncHooksContextManager().enable());
6262
mockServer.start(done);
@@ -105,8 +105,8 @@ describe('UndiciInstrumentation `fetch` tests', function () {
105105
let spans = memoryExporter.getFinishedSpans();
106106
assert.strictEqual(spans.length, 0);
107107

108-
// Disable via config
109-
instrumentation.setConfig({ enabled: false });
108+
// Disable
109+
instrumentation.disable();
110110

111111
const fetchUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`;
112112
const response = await fetch(fetchUrl);
@@ -126,7 +126,8 @@ describe('UndiciInstrumentation `fetch` tests', function () {
126126
});
127127
afterEach(function () {
128128
// Empty configuration & disable
129-
instrumentation.setConfig({ enabled: false });
129+
instrumentation.setConfig({});
130+
instrumentation.disable();
130131
});
131132

132133
it('should create valid spans even if the configuration hooks fail', async function () {
@@ -135,7 +136,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {
135136

136137
// Set the bad configuration
137138
instrumentation.setConfig({
138-
enabled: true,
139139
ignoreRequestHook: () => {
140140
throw new Error('ignoreRequestHook error');
141141
},
@@ -204,7 +204,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {
204204

205205
// Set configuration
206206
instrumentation.setConfig({
207-
enabled: true,
208207
ignoreRequestHook: req => {
209208
return req.path.indexOf('/ignore/path') !== -1;
210209
},
@@ -302,7 +301,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {
302301
assert.strictEqual(spans.length, 0);
303302

304303
instrumentation.setConfig({
305-
enabled: true,
306304
requireParentforSpans: true,
307305
});
308306

@@ -322,7 +320,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {
322320
assert.strictEqual(spans.length, 0);
323321

324322
instrumentation.setConfig({
325-
enabled: true,
326323
requireParentforSpans: true,
327324
});
328325

‎plugins/node/instrumentation-undici/test/metrics.test.ts‎

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -31,24 +31,19 @@ import { MockServer } from './utils/mock-server';
3131
import { MockMetricsReader } from './utils/mock-metrics-reader';
3232
import { SemanticAttributes } from '../src/enums/SemanticAttributes';
3333

34-
const instrumentation = new UndiciInstrumentation();
35-
instrumentation.enable();
36-
instrumentation.disable();
37-
38-
const protocol = 'http';
39-
const hostname = 'localhost';
40-
const mockServer = new MockServer();
41-
const provider = new NodeTracerProvider();
42-
const meterProvider = new MeterProvider();
43-
const metricsMemoryExporter = new InMemoryMetricExporter(
44-
AggregationTemporality.DELTA
45-
);
46-
const metricReader = new MockMetricsReader(metricsMemoryExporter);
47-
meterProvider.addMetricReader(metricReader);
48-
instrumentation.setTracerProvider(provider);
49-
instrumentation.setMeterProvider(meterProvider);
50-
5134
describe('UndiciInstrumentation metrics tests', function () {
35+
let instrumentation: UndiciInstrumentation;
36+
const protocol = 'http';
37+
const hostname = 'localhost';
38+
const mockServer = new MockServer();
39+
const provider = new NodeTracerProvider();
40+
const meterProvider = new MeterProvider();
41+
const metricsMemoryExporter = new InMemoryMetricExporter(
42+
AggregationTemporality.DELTA
43+
);
44+
const metricReader = new MockMetricsReader(metricsMemoryExporter);
45+
meterProvider.addMetricReader(metricReader);
46+
5247
before(function (done) {
5348
// Do not test if the `fetch` global API is not available
5449
// This applies to nodejs < v18 or nodejs < v16.15 without the flag
@@ -58,6 +53,10 @@ describe('UndiciInstrumentation metrics tests', function () {
5853
this.skip();
5954
}
6055

56+
instrumentation = new UndiciInstrumentation();
57+
instrumentation.setTracerProvider(provider);
58+
instrumentation.setMeterProvider(meterProvider);
59+
6160
context.setGlobalContextManager(new AsyncHooksContextManager().enable());
6261
mockServer.start(done);
6362
mockServer.mockListener((req, res) => {
@@ -67,13 +66,10 @@ describe('UndiciInstrumentation metrics tests', function () {
6766
res.write(JSON.stringify({ success: true }));
6867
res.end();
6968
});
70-
71-
// enable instrumentation for all tests
72-
instrumentation.enable();
7369
});
7470

7571
after(function (done) {
76-
instrumentation.disable();
72+
instrumentation?.disable();
7773
context.disable();
7874
propagation.disable();
7975
mockServer.mockListener(undefined);

‎plugins/node/instrumentation-undici/test/undici.test.ts‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -39,26 +39,6 @@ import { assertSpan } from './utils/assertSpan';
3939

4040
import type { fetch, stream, request, Client, Dispatcher } from 'undici';
4141

42-
const instrumentation = new UndiciInstrumentation();
43-
instrumentation.enable();
44-
instrumentation.disable();
45-
46-
// Reference to the `undici` module
47-
let undici: {
48-
fetch: typeof fetch;
49-
request: typeof request;
50-
stream: typeof stream;
51-
Client: typeof Client;
52-
};
53-
54-
const protocol = 'http';
55-
const hostname = 'localhost';
56-
const mockServer = new MockServer();
57-
const memoryExporter = new InMemorySpanExporter();
58-
const provider = new NodeTracerProvider();
59-
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));
60-
instrumentation.setTracerProvider(provider);
61-
6242
// Undici docs (https://github.com/nodejs/undici#garbage-collection) suggest
6343
// that an undici response body should always be consumed.
6444
async function consumeResponseBody(body: Dispatcher.ResponseData['body']) {
@@ -74,6 +54,23 @@ async function consumeResponseBody(body: Dispatcher.ResponseData['body']) {
7454
}
7555

7656
describe('UndiciInstrumentation `undici` tests', function () {
57+
let instrumentation: UndiciInstrumentation;
58+
59+
// Reference to the `undici` module
60+
let undici: {
61+
fetch: typeof fetch;
62+
request: typeof request;
63+
stream: typeof stream;
64+
Client: typeof Client;
65+
};
66+
67+
const protocol = 'http';
68+
const hostname = 'localhost';
69+
const mockServer = new MockServer();
70+
const memoryExporter = new InMemorySpanExporter();
71+
const provider = new NodeTracerProvider();
72+
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));
73+
7774
before(function (done) {
7875
// Load `undici`. It may fail if nodejs version is <18 because the module uses
7976
// features only available from that version. In that case skip the test.
@@ -83,6 +80,9 @@ describe('UndiciInstrumentation `undici` tests', function () {
8380
this.skip();
8481
}
8582

83+
instrumentation = new UndiciInstrumentation();
84+
instrumentation.setTracerProvider(provider);
85+
8686
propagation.setGlobalPropagator(new MockPropagation());
8787
context.setGlobalContextManager(new AsyncHooksContextManager().enable());
8888
mockServer.start(done);
@@ -136,7 +136,7 @@ describe('UndiciInstrumentation `undici` tests', function () {
136136
assert.strictEqual(spans.length, 0);
137137

138138
// Disable via config
139-
instrumentation.setConfig({ enabled: false });
139+
instrumentation.disable();
140140

141141
const requestUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`;
142142
const { headers, body } = await undici.request(requestUrl);
@@ -157,7 +157,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
157157
instrumentation.enable();
158158
// Set configuration
159159
instrumentation.setConfig({
160-
enabled: true,
161160
ignoreRequestHook: req => {
162161
return req.path.indexOf('/ignore/path') !== -1;
163162
},
@@ -188,7 +187,8 @@ describe('UndiciInstrumentation `undici` tests', function () {
188187
});
189188
afterEach(function () {
190189
// Empty configuration & disable
191-
instrumentation.setConfig({ enabled: false });
190+
instrumentation.setConfig({});
191+
instrumentation.disable();
192192
});
193193

194194
it('should ignore requests based on the result of ignoreRequestHook', async function () {
@@ -595,7 +595,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
595595

596596
// Set the bad configuration
597597
instrumentation.setConfig({
598-
enabled: true,
599598
ignoreRequestHook: () => {
600599
throw new Error('ignoreRequestHook error');
601600
},
@@ -639,7 +638,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
639638
assert.strictEqual(spans.length, 0);
640639

641640
instrumentation.setConfig({
642-
enabled: true,
643641
requireParentforSpans: true,
644642
});
645643

@@ -661,7 +659,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
661659
assert.strictEqual(spans.length, 0);
662660

663661
instrumentation.setConfig({
664-
enabled: true,
665662
requireParentforSpans: true,
666663
});
667664

@@ -685,7 +682,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
685682
assert.strictEqual(spans.length, 0);
686683

687684
instrumentation.setConfig({
688-
enabled: true,
689685
requireParentforSpans: true,
690686
});
691687

0 commit comments

Comments
 (0)