Skip to content

Commit

Permalink
fix(instr-undici): fix issue with config in constructor (#2395)
Browse files Browse the repository at this point in the history 
Co-authored-by: Trent Mick <trentm@gmail.com>
  • Loading branch information
@david-luna @trentm
david-luna and trentm authored Aug 22, 2024
1 parent 7054bc1 commit ca70bb9
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 91 deletions.
44 changes: 20 additions & 24 deletions plugins/node/instrumentation-undici/src/undici.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
private _httpClientDurationHistogram!: Histogram;
constructor(config: UndiciInstrumentationConfig = {}) {
super(PACKAGE_NAME, PACKAGE_VERSION, config);
this.setConfig(config);
}

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

override disable(): void {
if (!this.getConfig().enabled) {
return;
}

super.disable();
this._channelSubs.forEach(sub => 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)
Expand All @@ -113,16 +118,6 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
this.subscribeToChannel('undici:request:error', this.onError.bind(this));
}

override setConfig(config: UndiciInstrumentationConfig = {}): void {
super.setConfig(config);

if (config?.enabled) {
this.enable();
} else {
this.disable();
}
}

protected override _updateMetricInstruments() {
this._httpClientDurationHistogram = this.meter.createHistogram(
'http.client.request.duration',
Expand Down Expand Up @@ -162,9 +157,10 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
// - ignored by config
// - method is 'CONNECT'
const config = this.getConfig();
const enabled = config.enabled !== false;
const shouldIgnoreReq = safeExecuteInTheMiddle(
() =>
!config.enabled ||
!enabled ||
request.method === 'CONNECT' ||
config.ignoreRequestHook?.(request),
e => e && this._diag.error('caught ignoreRequestHook error: ', e),
Expand Down
35 changes: 16 additions & 19 deletions plugins/node/instrumentation-undici/test/fetch.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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 () {
Expand All @@ -135,7 +136,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {

// Set the bad configuration
instrumentation.setConfig({
enabled: true,
ignoreRequestHook: () => {
throw new Error('ignoreRequestHook error');
},
Expand Down Expand Up @@ -204,7 +204,6 @@ describe('UndiciInstrumentation `fetch` tests', function () {

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

instrumentation.setConfig({
enabled: true,
requireParentforSpans: true,
});

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

instrumentation.setConfig({
enabled: true,
requireParentforSpans: true,
});

Expand Down
38 changes: 17 additions & 21 deletions plugins/node/instrumentation-undici/test/metrics.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) => {
Expand All @@ -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);
Expand Down
50 changes: 23 additions & 27 deletions plugins/node/instrumentation-undici/test/undici.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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']) {
Expand All @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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;
},
Expand Down Expand Up @@ -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 () {
Expand Down Expand Up @@ -595,7 +595,6 @@ describe('UndiciInstrumentation `undici` tests', function () {

// Set the bad configuration
instrumentation.setConfig({
enabled: true,
ignoreRequestHook: () => {
throw new Error('ignoreRequestHook error');
},
Expand Down Expand Up @@ -639,7 +638,6 @@ describe('UndiciInstrumentation `undici` tests', function () {
assert.strictEqual(spans.length, 0);

instrumentation.setConfig({
enabled: true,
requireParentforSpans: true,
});

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

instrumentation.setConfig({
enabled: true,
requireParentforSpans: true,
});

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

instrumentation.setConfig({
enabled: true,
requireParentforSpans: true,
});

Expand Down

0 comments on commit ca70bb9

Please sign in to comment.