Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(instr-undici): fix issue with config in constructor #2395

Merged
merged 6 commits into from
Aug 22, 2024
Merged

fix(instr-undici): fix issue with config in constructor #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6dc2736
fix(instr-undici): fix issue with config in constructor
david-luna Aug 21, 2024
c928cdd
chore(instr-undici): fix lint issues
david-luna Aug 21, 2024
1fd1cc2
chore(instr-undici): avoid dupicate subscriptions
david-luna Aug 21, 2024
30cc3ef
chore(instr-undici): fix mertics test for node v14 & v16
david-luna Aug 21, 2024
2ed4802
restore the `super.` calls for enable/disable to InstrumentationBase'…
trentm Aug 21, 2024
e473df6
lint:fix
trentm Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 20 additions & 24 deletions plugins/node/instrumentation-undici/src/undici.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@
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 @@
}

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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: no need to call super since the parent class method deals with patching/unpatching and does not apply here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super.disable() is also setting this._enabled = false, so I wonder if it still should be called.
Those calls to super. in the enable() and disable() were added in this PR: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2289/files#diff-8e6dd9963dd47a60e25c9ac92d7a5ae25ce79c40340cb4721eefd203bec81a60R86

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've restored the super.disable() and super.enable() calls, to allow InstrumentationBase to maintain its internal this._enabled setting.

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;

Check warning on line 102 in plugins/node/instrumentation-undici/src/undici.ts

View check run for this annotation

Codecov / codecov/patch

plugins/node/instrumentation-undici/src/undici.ts#L102 

Added line #L102 was not covered by tests
}

this.subscribeToChannel(
'undici:request:create',
this.onRequestCreated.bind(this)
Expand All @@ -113,16 +118,6 @@
this.subscribeToChannel('undici:request:error', this.onError.bind(this));
}

override setConfig(config: UndiciInstrumentationConfig = {}): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: here I was mixing UndiciInstrumentationConfig with InstrumentationConfig. This side effect shouldn't be there

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 @@
// - 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
Open in desktop
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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: the test runner was loading all the test suites at once and also evaluating the 1st describe block. As a consequence there were 3 instances of the instrumentation before any test was ran. Leading to errors because all instances where getting messages from diagnostics channels

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

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
Open in desktop
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
Open in desktop
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