diff --git a/plugins/node/instrumentation-undici/src/undici.ts b/plugins/node/instrumentation-undici/src/undici.ts index 1d5445d60b..0d8ebedc73 100644 --- a/plugins/node/instrumentation-undici/src/undici.ts +++ b/plugins/node/instrumentation-undici/src/undici.ts @@ -68,7 +68,6 @@ export class UndiciInstrumentation extends InstrumentationBase sub.channel.unsubscribe(sub.onMessage)); this._channelSubs.length = 0; - super.disable(); - this.setConfig({ ...this.getConfig(), enabled: false }); } override enable(): void { - if (this.getConfig().enabled) { - return; - } + // "enabled" handling is currently a bit messy with InstrumentationBase. + // If constructed with `{enabled: false}`, this `.enable()` is still called, + // and `this.getConfig().enabled !== this.isEnabled()`, creating confusion. + // + // For now, this class will setup for instrumenting if `.enable()` is + // called, but use `this.getConfig().enabled` to determine if + // instrumentation should be generated. This covers the more likely common + // case of config being given a construction time, rather than later via + // `instance.enable()`, `.disable()`, or `.setConfig()` calls. super.enable(); - this.setConfig({ ...this.getConfig(), enabled: true }); - // This method is called by the `InstrumentationAbstract` constructor before - // ours is called. So we need to ensure the property is initalized + // This method is called by the super-class constructor before ours is + // called. So we need to ensure the property is initalized. this._channelSubs = this._channelSubs || []; + + // Avoid to duplicate subscriptions + if (this._channelSubs.length > 0) { + return; + } + this.subscribeToChannel( 'undici:request:create', this.onRequestCreated.bind(this) @@ -113,16 +118,6 @@ export class UndiciInstrumentation extends InstrumentationBase - !config.enabled || + !enabled || request.method === 'CONNECT' || config.ignoreRequestHook?.(request), e => e && this._diag.error('caught ignoreRequestHook error: ', e), diff --git a/plugins/node/instrumentation-undici/test/fetch.test.ts b/plugins/node/instrumentation-undici/test/fetch.test.ts index fc369edead..9ef4e90767 100644 --- a/plugins/node/instrumentation-undici/test/fetch.test.ts +++ b/plugins/node/instrumentation-undici/test/fetch.test.ts @@ -35,19 +35,16 @@ import { MockPropagation } from './utils/mock-propagation'; import { MockServer } from './utils/mock-server'; import { assertSpan } from './utils/assertSpan'; -const instrumentation = new UndiciInstrumentation(); -instrumentation.enable(); -instrumentation.disable(); - -const protocol = 'http'; -const hostname = 'localhost'; -const mockServer = new MockServer(); -const memoryExporter = new InMemorySpanExporter(); -const provider = new NodeTracerProvider(); -provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); -instrumentation.setTracerProvider(provider); - describe('UndiciInstrumentation `fetch` tests', function () { + let instrumentation: UndiciInstrumentation; + + const protocol = 'http'; + const hostname = 'localhost'; + const mockServer = new MockServer(); + const memoryExporter = new InMemorySpanExporter(); + const provider = new NodeTracerProvider(); + provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); + before(function (done) { // Do not test if the `fetch` global API is not available // This applies to nodejs < v18 or nodejs < v16.15 wihtout the flag @@ -57,6 +54,9 @@ describe('UndiciInstrumentation `fetch` tests', function () { this.skip(); } + instrumentation = new UndiciInstrumentation(); + instrumentation.setTracerProvider(provider); + propagation.setGlobalPropagator(new MockPropagation()); context.setGlobalContextManager(new AsyncHooksContextManager().enable()); mockServer.start(done); @@ -105,8 +105,8 @@ describe('UndiciInstrumentation `fetch` tests', function () { let spans = memoryExporter.getFinishedSpans(); assert.strictEqual(spans.length, 0); - // Disable via config - instrumentation.setConfig({ enabled: false }); + // Disable + instrumentation.disable(); const fetchUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`; const response = await fetch(fetchUrl); @@ -126,7 +126,8 @@ describe('UndiciInstrumentation `fetch` tests', function () { }); afterEach(function () { // Empty configuration & disable - instrumentation.setConfig({ enabled: false }); + instrumentation.setConfig({}); + instrumentation.disable(); }); it('should create valid spans even if the configuration hooks fail', async function () { @@ -135,7 +136,6 @@ describe('UndiciInstrumentation `fetch` tests', function () { // Set the bad configuration instrumentation.setConfig({ - enabled: true, ignoreRequestHook: () => { throw new Error('ignoreRequestHook error'); }, @@ -204,7 +204,6 @@ describe('UndiciInstrumentation `fetch` tests', function () { // Set configuration instrumentation.setConfig({ - enabled: true, ignoreRequestHook: req => { return req.path.indexOf('/ignore/path') !== -1; }, @@ -302,7 +301,6 @@ describe('UndiciInstrumentation `fetch` tests', function () { assert.strictEqual(spans.length, 0); instrumentation.setConfig({ - enabled: true, requireParentforSpans: true, }); @@ -322,7 +320,6 @@ describe('UndiciInstrumentation `fetch` tests', function () { assert.strictEqual(spans.length, 0); instrumentation.setConfig({ - enabled: true, requireParentforSpans: true, }); diff --git a/plugins/node/instrumentation-undici/test/metrics.test.ts b/plugins/node/instrumentation-undici/test/metrics.test.ts index 0fc633ef19..a903698385 100644 --- a/plugins/node/instrumentation-undici/test/metrics.test.ts +++ b/plugins/node/instrumentation-undici/test/metrics.test.ts @@ -31,24 +31,19 @@ import { MockServer } from './utils/mock-server'; import { MockMetricsReader } from './utils/mock-metrics-reader'; import { SemanticAttributes } from '../src/enums/SemanticAttributes'; -const instrumentation = new UndiciInstrumentation(); -instrumentation.enable(); -instrumentation.disable(); - -const protocol = 'http'; -const hostname = 'localhost'; -const mockServer = new MockServer(); -const provider = new NodeTracerProvider(); -const meterProvider = new MeterProvider(); -const metricsMemoryExporter = new InMemoryMetricExporter( - AggregationTemporality.DELTA -); -const metricReader = new MockMetricsReader(metricsMemoryExporter); -meterProvider.addMetricReader(metricReader); -instrumentation.setTracerProvider(provider); -instrumentation.setMeterProvider(meterProvider); - describe('UndiciInstrumentation metrics tests', function () { + let instrumentation: UndiciInstrumentation; + const protocol = 'http'; + const hostname = 'localhost'; + const mockServer = new MockServer(); + const provider = new NodeTracerProvider(); + const meterProvider = new MeterProvider(); + const metricsMemoryExporter = new InMemoryMetricExporter( + AggregationTemporality.DELTA + ); + const metricReader = new MockMetricsReader(metricsMemoryExporter); + meterProvider.addMetricReader(metricReader); + before(function (done) { // Do not test if the `fetch` global API is not available // This applies to nodejs < v18 or nodejs < v16.15 without the flag @@ -58,6 +53,10 @@ describe('UndiciInstrumentation metrics tests', function () { this.skip(); } + instrumentation = new UndiciInstrumentation(); + instrumentation.setTracerProvider(provider); + instrumentation.setMeterProvider(meterProvider); + context.setGlobalContextManager(new AsyncHooksContextManager().enable()); mockServer.start(done); mockServer.mockListener((req, res) => { @@ -67,13 +66,10 @@ describe('UndiciInstrumentation metrics tests', function () { res.write(JSON.stringify({ success: true })); res.end(); }); - - // enable instrumentation for all tests - instrumentation.enable(); }); after(function (done) { - instrumentation.disable(); + instrumentation?.disable(); context.disable(); propagation.disable(); mockServer.mockListener(undefined); diff --git a/plugins/node/instrumentation-undici/test/undici.test.ts b/plugins/node/instrumentation-undici/test/undici.test.ts index b0bd088199..451bb71f3f 100644 --- a/plugins/node/instrumentation-undici/test/undici.test.ts +++ b/plugins/node/instrumentation-undici/test/undici.test.ts @@ -39,26 +39,6 @@ import { assertSpan } from './utils/assertSpan'; import type { fetch, stream, request, Client, Dispatcher } from 'undici'; -const instrumentation = new UndiciInstrumentation(); -instrumentation.enable(); -instrumentation.disable(); - -// Reference to the `undici` module -let undici: { - fetch: typeof fetch; - request: typeof request; - stream: typeof stream; - Client: typeof Client; -}; - -const protocol = 'http'; -const hostname = 'localhost'; -const mockServer = new MockServer(); -const memoryExporter = new InMemorySpanExporter(); -const provider = new NodeTracerProvider(); -provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); -instrumentation.setTracerProvider(provider); - // Undici docs (https://github.com/nodejs/undici#garbage-collection) suggest // that an undici response body should always be consumed. async function consumeResponseBody(body: Dispatcher.ResponseData['body']) { @@ -74,6 +54,23 @@ async function consumeResponseBody(body: Dispatcher.ResponseData['body']) { } describe('UndiciInstrumentation `undici` tests', function () { + let instrumentation: UndiciInstrumentation; + + // Reference to the `undici` module + let undici: { + fetch: typeof fetch; + request: typeof request; + stream: typeof stream; + Client: typeof Client; + }; + + const protocol = 'http'; + const hostname = 'localhost'; + const mockServer = new MockServer(); + const memoryExporter = new InMemorySpanExporter(); + const provider = new NodeTracerProvider(); + provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); + before(function (done) { // Load `undici`. It may fail if nodejs version is <18 because the module uses // features only available from that version. In that case skip the test. @@ -83,6 +80,9 @@ describe('UndiciInstrumentation `undici` tests', function () { this.skip(); } + instrumentation = new UndiciInstrumentation(); + instrumentation.setTracerProvider(provider); + propagation.setGlobalPropagator(new MockPropagation()); context.setGlobalContextManager(new AsyncHooksContextManager().enable()); mockServer.start(done); @@ -136,7 +136,7 @@ describe('UndiciInstrumentation `undici` tests', function () { assert.strictEqual(spans.length, 0); // Disable via config - instrumentation.setConfig({ enabled: false }); + instrumentation.disable(); const requestUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`; const { headers, body } = await undici.request(requestUrl); @@ -157,7 +157,6 @@ describe('UndiciInstrumentation `undici` tests', function () { instrumentation.enable(); // Set configuration instrumentation.setConfig({ - enabled: true, ignoreRequestHook: req => { return req.path.indexOf('/ignore/path') !== -1; }, @@ -188,7 +187,8 @@ describe('UndiciInstrumentation `undici` tests', function () { }); afterEach(function () { // Empty configuration & disable - instrumentation.setConfig({ enabled: false }); + instrumentation.setConfig({}); + instrumentation.disable(); }); it('should ignore requests based on the result of ignoreRequestHook', async function () { @@ -595,7 +595,6 @@ describe('UndiciInstrumentation `undici` tests', function () { // Set the bad configuration instrumentation.setConfig({ - enabled: true, ignoreRequestHook: () => { throw new Error('ignoreRequestHook error'); }, @@ -639,7 +638,6 @@ describe('UndiciInstrumentation `undici` tests', function () { assert.strictEqual(spans.length, 0); instrumentation.setConfig({ - enabled: true, requireParentforSpans: true, }); @@ -661,7 +659,6 @@ describe('UndiciInstrumentation `undici` tests', function () { assert.strictEqual(spans.length, 0); instrumentation.setConfig({ - enabled: true, requireParentforSpans: true, }); @@ -685,7 +682,6 @@ describe('UndiciInstrumentation `undici` tests', function () { assert.strictEqual(spans.length, 0); instrumentation.setConfig({ - enabled: true, requireParentforSpans: true, });