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.

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

feat(http): Allow to opt-out of instrumenting incoming/outgoing requests #4643

Merged
merged 5 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* feat(instrumentation-http): Allow to opt-out of instrumenting incoming/outgoing requests [#4643](https://github.com/open-telemetry/opentelemetry-js/pull/4643) @mydea

### :bug: (Bug Fix)

* fix(instrumentation-http): Ensure instrumentation of `http.get` and `https.get` work when used in ESM code [#4857](https://github.com/open-telemetry/opentelemetry-js/issues/4857) @trentm
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ Http instrumentation has few options available to choose from. You can set the f
| [`startOutgoingSpanHook`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L99) | `StartOutgoingSpanCustomAttributeFunction` | Function for adding custom attributes before a span is started in outgoingRequest |
| `ignoreIncomingRequestHook` | `IgnoreIncomingRequestFunction` | Http instrumentation will not trace all incoming requests that matched with custom function |
| `ignoreOutgoingRequestHook` | `IgnoreOutgoingRequestFunction` | Http instrumentation will not trace all outgoing requests that matched with custom function |
| `disableOutgoingRequestInstrumentation` | `boolean` | Set to true to avoid instrumenting outgoing requests at all. This can be helpful when another instrumentation handles outgoing requests. |
| `disableIncomingRequestInstrumentation` | `boolean` | Set to true to avoid instrumenting incoming requests at all. This can be helpful when another instrumentation handles incoming requests. |
| [`serverName`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L101) | `string` | The primary server name of the matched virtual host. |
| [`requireParentforOutgoingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L103) | Boolean | Require that is a parent span to create new span for outgoing requests. |
| [`requireParentforIncomingSpans`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L105) | Boolean | Require that is a parent span to create new span for incoming requests. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,29 +110,37 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
'http',
['*'],
(moduleExports: Http): Http => {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchOutgoingRequestFunction('http')
) as unknown as Func<http.ClientRequest>;
this._wrap(
moduleExports,
'get',
this._getPatchOutgoingGetFunction(patchedRequest)
);
this._wrap(
moduleExports.Server.prototype,
'emit',
this._getPatchIncomingRequestFunction('http')
);
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchOutgoingRequestFunction('http')
) as unknown as Func<http.ClientRequest>;
this._wrap(
moduleExports,
'get',
this._getPatchOutgoingGetFunction(patchedRequest)
);
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._wrap(
moduleExports.Server.prototype,
'emit',
this._getPatchIncomingRequestFunction('http')
);
}
return moduleExports;
},
(moduleExports: Http) => {
if (moduleExports === undefined) return;

this._unwrap(moduleExports, 'request');
this._unwrap(moduleExports, 'get');
this._unwrap(moduleExports.Server.prototype, 'emit');
if (!this.getConfig().disableOutgoingRequestInstrumentation) {

Choose a reason for hiding this comment

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

wouldn't this be better using the isWrapped check ?

if (isWrapped(module.request)) {
this._unwrap(module, 'request')
}

this._unwrap(moduleExports, 'request');
this._unwrap(moduleExports, 'get');
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._unwrap(moduleExports.Server.prototype, 'emit');
}
}
);
}
Expand All @@ -142,30 +150,37 @@ export class HttpInstrumentation extends InstrumentationBase<HttpInstrumentation
'https',
['*'],
(moduleExports: Https): Https => {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchHttpsOutgoingRequestFunction('https')
) as unknown as Func<http.ClientRequest>;
this._wrap(
moduleExports,
'get',
this._getPatchHttpsOutgoingGetFunction(patchedRequest)
);

this._wrap(
moduleExports.Server.prototype,
'emit',
this._getPatchIncomingRequestFunction('https')
);
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
const patchedRequest = this._wrap(
moduleExports,
'request',
this._getPatchHttpsOutgoingRequestFunction('https')
) as unknown as Func<http.ClientRequest>;
this._wrap(
moduleExports,
'get',
this._getPatchHttpsOutgoingGetFunction(patchedRequest)
);
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._wrap(
moduleExports.Server.prototype,
'emit',
this._getPatchIncomingRequestFunction('https')
);
}
return moduleExports;
},
(moduleExports: Https) => {
if (moduleExports === undefined) return;

this._unwrap(moduleExports, 'request');
this._unwrap(moduleExports, 'get');
this._unwrap(moduleExports.Server.prototype, 'emit');
if (!this.getConfig().disableOutgoingRequestInstrumentation) {
this._unwrap(moduleExports, 'request');
this._unwrap(moduleExports, 'get');
}
if (!this.getConfig().disableIncomingRequestInstrumentation) {
this._unwrap(moduleExports.Server.prototype, 'emit');
}
}
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ export interface HttpInstrumentationConfig extends InstrumentationConfig {
ignoreOutgoingUrls?: IgnoreMatcher[];
/** Not trace all outgoing requests that matched with custom function */
ignoreOutgoingRequestHook?: IgnoreOutgoingRequestFunction;
/** If set to true, incoming requests will not be instrumented at all. */
disableIncomingRequestInstrumentation?: boolean;
/** If set to true, outgoing requests will not be instrumented at all. */
disableOutgoingRequestInstrumentation?: boolean;
/** Function for adding custom attributes after response is handled */
applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
/** Function for adding custom attributes before request is handled */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,55 @@ describe('HttpInstrumentation', () => {
});
});

describe('partially disable instrumentation', () => {
beforeEach(() => {
memoryExporter.reset();
});

afterEach(() => {
server.close();
instrumentation.disable();
});

it('allows to disable outgoing request instrumentation', () => {
server.close();
instrumentation.disable();

instrumentation.setConfig({
disableOutgoingRequestInstrumentation: true,
});
instrumentation.enable();
server = http.createServer((_request, response) => {
response.end('Test Server Response');
});

server.listen(serverPort);

assert.strictEqual(isWrapped(http.Server.prototype.emit), true);
assert.strictEqual(isWrapped(http.get), false);
assert.strictEqual(isWrapped(http.request), false);
});

it('allows to disable incoming request instrumentation', () => {
server.close();
instrumentation.disable();

instrumentation.setConfig({
disableIncomingRequestInstrumentation: true,
});
instrumentation.enable();
server = http.createServer((_request, response) => {
response.end('Test Server Response');
});

server.listen(serverPort);

assert.strictEqual(isWrapped(http.Server.prototype.emit), false);
assert.strictEqual(isWrapped(http.get), true);
assert.strictEqual(isWrapped(http.request), true);
});
});

describe('with good instrumentation options', () => {
beforeEach(() => {
memoryExporter.reset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ describe('HttpsInstrumentation', () => {
);
});
});

describe('with good instrumentation options', () => {
beforeEach(() => {
memoryExporter.reset();
Expand Down Expand Up @@ -704,5 +705,60 @@ describe('HttpsInstrumentation', () => {
req.end();
});
});

describe('partially disable instrumentation', () => {
beforeEach(() => {
memoryExporter.reset();
});

afterEach(() => {
server.close();
instrumentation.disable();
});

it('allows to disable outgoing request instrumentation', () => {
instrumentation.setConfig({
disableOutgoingRequestInstrumentation: true,
});
instrumentation.enable();
server = https.createServer(
{
key: fs.readFileSync('test/fixtures/server-key.pem'),
cert: fs.readFileSync('test/fixtures/server-cert.pem'),
},
(request, response) => {
response.end('Test Server Response');
}
);

server.listen(serverPort);

assert.strictEqual(isWrapped(http.Server.prototype.emit), true);
assert.strictEqual(isWrapped(http.get), false);
assert.strictEqual(isWrapped(http.request), false);
});

it('allows to disable incoming request instrumentation', () => {
instrumentation.setConfig({
disableIncomingRequestInstrumentation: true,
});
instrumentation.enable();
server = https.createServer(
{
key: fs.readFileSync('test/fixtures/server-key.pem'),
cert: fs.readFileSync('test/fixtures/server-cert.pem'),
},
(request, response) => {
response.end('Test Server Response');
}
);

server.listen(serverPort);

assert.strictEqual(isWrapped(http.Server.prototype.emit), false);
assert.strictEqual(isWrapped(http.get), true);
assert.strictEqual(isWrapped(http.request), true);
});
});
});
});