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(instrumentation-http): add options to ignore requests #2704

Merged
merged 8 commits into from
Jan 17, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ Http instrumentation has few options available to choose from. You can set the f
| [`startIncomingSpanHook`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L97) | `StartIncomingSpanCustomAttributeFunction` | Function for adding custom attributes before a span is started in incomingRequest |
| [`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 |
| [`ignoreIncomingPaths`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L87) | `IgnoreMatcher[]` | Http instrumentation will not trace all incoming requests that match paths |
Copy link
Member

Choose a reason for hiding this comment

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

now that we have a general hook, should we deprecate these options, and consider removing them on the next breaking change (1.0.0)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with deprecating it. But I don't have a strong feeling about it. What do other folks think about it?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it too, its preferable to have one generic option over multiple specifics

Copy link
Member

Choose a reason for hiding this comment

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

agreed, nice generic approach 👍 i did the same for pino-http: pinojs/pino-http#153

| `ignoreIncomingRequestHook` | `IgnoreIncomingRequestFunction[]` | Http instrumentation will not trace all incoming requests that matched with custom function |
legendecas marked this conversation as resolved.
Show resolved Hide resolved
| [`ignoreOutgoingUrls`](https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/types.ts#L89) | `IgnoreMatcher[]` | Http instrumentation will not trace all outgoing requests that match urls |
| `ignoreOutgoingRequestHook` | `IgnoreOutgoingRequestFunction[]` | Http instrumentation will not trace all outgoing requests that matched with custom function |
legendecas marked this conversation as resolved.
Show resolved Hide resolved
| [`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 @@ -382,7 +382,12 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
utils.isIgnored(
pathname,
instrumentation._getConfig().ignoreIncomingPaths,
(e: Error) => instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
(e: unknown) => instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
) ||
safeExecuteInTheMiddle(
() => instrumentation._getConfig().ignoreIncomingRequestHook?.(request),
() => {},
legendecas marked this conversation as resolved.
Show resolved Hide resolved
true
)
) {
return context.with(suppressTracing(context.active()), () => {
Expand Down Expand Up @@ -534,7 +539,12 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
utils.isIgnored(
origin + pathname,
instrumentation._getConfig().ignoreOutgoingUrls,
(e: Error) => instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
(e: unknown) => instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
) ||
safeExecuteInTheMiddle(
() => instrumentation._getConfig().ignoreOutgoingRequestHook?.(optionsParsed),
() => {},
true
)
) {
return original.apply(this, [optionsParsed, ...args]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import {
import {
Span,
SpanAttributes,
} from '@opentelemetry/api';
Expand Down Expand Up @@ -63,6 +63,14 @@ export interface HttpCustomAttributeFunction {
): void;
}

export interface IgnoreIncomingRequestFunction {
(request: IncomingMessage ): boolean;
}

export interface IgnoreOutgoingRequestFunction {
(request: RequestOptions ): boolean;
}

export interface HttpRequestCustomAttributeFunction {
(span: Span, request: ClientRequest | IncomingMessage): void;
}
Expand All @@ -85,8 +93,12 @@ export interface StartOutgoingSpanCustomAttributeFunction {
export interface HttpInstrumentationConfig extends InstrumentationConfig {
/** Not trace all incoming requests that match paths */
ignoreIncomingPaths?: IgnoreMatcher[];
/** Not trace all incoming requests that matched with custom function */
ignoreIncomingRequestHook?: IgnoreIncomingRequestFunction;
/** Not trace all outgoing requests that match urls */
ignoreOutgoingUrls?: IgnoreMatcher[];
/** Not trace all outgoing requests that matched with custom function */
ignoreOutgoingRequestHook?: IgnoreOutgoingRequestFunction;
/** 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 @@ -129,7 +129,7 @@ export const satisfiesPattern = (
export const isIgnored = (
constant: string,
list?: IgnoreMatcher[],
onException?: (error: Error) => void
onException?: (error: unknown) => void
): boolean => {
if (!list) {
// No ignored urls - trace everything
Expand Down Expand Up @@ -304,18 +304,18 @@ export const getRequestInfo = (
}`;
}

if (hasExpectHeader(optionsParsed)) {
legendecas marked this conversation as resolved.
Show resolved Hide resolved
optionsParsed.headers = Object.assign({}, optionsParsed.headers);
} else if (!optionsParsed.headers) {
optionsParsed.headers = {};
}
const headers = optionsParsed.headers ?? {};
optionsParsed.headers = Object.keys(headers).reduce((normalizedHeader, key) => {
normalizedHeader[key.toLowerCase()] = headers[key];
return normalizedHeader;
}, {} as OutgoingHttpHeaders);
// some packages return method in lowercase..
// ensure upperCase for consistency
const method = optionsParsed.method
? optionsParsed.method.toUpperCase()
: 'GET';

return { origin, pathname, method, optionsParsed };
return { origin, pathname, method, optionsParsed, };
};

/**
Expand Down Expand Up @@ -501,7 +501,7 @@ export function headerCapture(type: 'request' | 'response', headers: string[]) {
return (span: Span, getHeader: (key: string) => undefined | string | string[] | number) => {
for (const [capturedHeader, normalizedHeader] of normalizedHeaders) {
const value = getHeader(capturedHeader);

if (value === undefined) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
context,
propagation,
Span as ISpan,
SpanKind,
SpanKind,
trace,
SpanAttributes,
} from '@opentelemetry/api';
Expand Down Expand Up @@ -142,11 +142,17 @@ describe('HttpInstrumentation', () => {
throw new Error('bad ignoreIncomingPaths function');
},
],
ignoreIncomingRequestHook: _request => {
throw new Error('bad ignoreIncomingRequestHook function');
},
ignoreOutgoingUrls: [
(url: string) => {
throw new Error('bad ignoreOutgoingUrls function');
},
],
ignoreOutgoingRequestHook: _request => {
throw new Error('bad ignoreOutgoingRequestHook function');
},
applyCustomAttributesOnSpan: () => {
throw new Error(applyCustomAttributesOnSpanErrorMessage);
},
Expand All @@ -167,7 +173,12 @@ describe('HttpInstrumentation', () => {

it('should generate valid spans (client side and server side)', async () => {
const result = await httpRequest.get(
`${protocol}://${hostname}:${serverPort}${pathname}`
`${protocol}://${hostname}:${serverPort}${pathname}`,
{
headers: {
'user-agent': 'tester'
}
}
blumamir marked this conversation as resolved.
Show resolved Hide resolved
);
const spans = memoryExporter.getFinishedSpans();
const [incomingSpan, outgoingSpan] = spans;
Expand Down Expand Up @@ -207,11 +218,20 @@ describe('HttpInstrumentation', () => {
/\/ignored\/regexp$/i,
(url: string) => url.endsWith('/ignored/function'),
],
ignoreIncomingRequestHook: request => {
return request.headers['user-agent']?.match('ignored-string') != null;
},
ignoreOutgoingUrls: [
`${protocol}://${hostname}:${serverPort}/ignored/string`,
/\/ignored\/regexp$/i,
(url: string) => url.endsWith('/ignored/function'),
],
ignoreOutgoingRequestHook: request => {
if (request.headers?.['user-agent'] != null) {
return `${request.headers['user-agent']}`.match('ignored-string') != null;
}
return false;
},
applyCustomAttributesOnSpan: customAttributeFunction,
requestHook: requestHookFunction,
responseHook: responseHookFunction,
Expand Down Expand Up @@ -447,7 +467,7 @@ describe('HttpInstrumentation', () => {
});

for (const ignored of ['string', 'function', 'regexp']) {
it(`should not trace ignored requests (client and server side) with type ${ignored}`, async () => {
it(`should not trace ignored requests with paths (client and server side) with type ${ignored}`, async () => {
const testPath = `/ignored/${ignored}`;

await httpRequest.get(
Expand All @@ -458,6 +478,31 @@ describe('HttpInstrumentation', () => {
});
}

it('should not trace ignored requests with headers (client and server side)', async () => {
legendecas marked this conversation as resolved.
Show resolved Hide resolved
const testValue = 'ignored-string';

await Promise.all([
httpRequest.get(
`${protocol}://${hostname}:${serverPort}`,
{
headers: {
'user-agent': testValue
}
}
),
httpRequest.get(
`${protocol}://${hostname}:${serverPort}`,
{
headers: {
'uSeR-aGeNt': testValue
}
}
)
]);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 0);
});

for (const arg of ['string', {}, new Date()]) {
it(`should be tracable and not throw exception in ${protocol} instrumentation when passing the following argument ${JSON.stringify(
arg
Expand Down Expand Up @@ -507,7 +552,7 @@ describe('HttpInstrumentation', () => {
hostname: 'localhost',
pathname: '/',
forceStatus: {
code: SpanStatusCode.ERROR,
code: SpanStatusCode.ERROR,
message: err.message,
},
component: 'http',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,17 @@ describe('HttpsInstrumentation', () => {
throw new Error('bad ignoreIncomingPaths function');
},
],
ignoreIncomingRequestHook: _request => {
throw new Error('bad ignoreIncomingRequestHook function');
},
ignoreOutgoingUrls: [
(url: string) => {
throw new Error('bad ignoreOutgoingUrls function');
},
],
ignoreOutgoingRequestHook: _request => {
throw new Error('bad ignoreOutgoingRequestHook function');
},
applyCustomAttributesOnSpan: () => {
throw new Error(applyCustomAttributesOnSpanErrorMessage);
},
Expand All @@ -142,7 +148,12 @@ describe('HttpsInstrumentation', () => {

it('should generate valid spans (client side and server side)', async () => {
const result = await httpsRequest.get(
`${protocol}://${hostname}:${serverPort}${pathname}`
`${protocol}://${hostname}:${serverPort}${pathname}`,
{
headers: {
'user-agent': 'tester'
}
}
);
const spans = memoryExporter.getFinishedSpans();
const [incomingSpan, outgoingSpan] = spans;
Expand Down Expand Up @@ -181,11 +192,20 @@ describe('HttpsInstrumentation', () => {
/\/ignored\/regexp$/i,
(url: string) => url.endsWith('/ignored/function'),
],
ignoreIncomingRequestHook: request => {
return request.headers['user-agent']?.match('ignored-string') != null;
},
ignoreOutgoingUrls: [
`${protocol}://${hostname}:${serverPort}/ignored/string`,
/\/ignored\/regexp$/i,
(url: string) => url.endsWith('/ignored/function'),
],
ignoreOutgoingRequestHook: request => {
if (request.headers?.['user-agent'] != null) {
return `${request.headers['user-agent']}`.match('ignored-string') != null;
}
return false;
},
applyCustomAttributesOnSpan: customAttributeFunction,
serverName,
});
Expand Down Expand Up @@ -412,7 +432,7 @@ describe('HttpsInstrumentation', () => {
});

for (const ignored of ['string', 'function', 'regexp']) {
it(`should not trace ignored requests (client and server side) with type ${ignored}`, async () => {
it(`should not trace ignored requests with paths (client and server side) with type ${ignored}`, async () => {
const testPath = `/ignored/${ignored}`;

await httpsRequest.get(
Expand All @@ -423,6 +443,31 @@ describe('HttpsInstrumentation', () => {
});
}

it('should not trace ignored requests with headers (client and server side)', async () => {
const testValue = 'ignored-string';

await Promise.all([
httpsRequest.get(
`${protocol}://${hostname}:${serverPort}`,
{
headers: {
'user-agent': testValue
}
}
),
httpsRequest.get(
`${protocol}://${hostname}:${serverPort}`,
{
headers: {
'uSeR-aGeNt': testValue
}
}
)
]);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 0);
});

for (const arg of ['string', {}, new Date()]) {
it(`should be tracable and not throw exception in ${protocol} instrumentation when passing the following argument ${JSON.stringify(
arg
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ describe('Utility', () => {
});

it('should not re-throw when function throws an exception', () => {
const onException = (e: Error) => {
const onException = (e: unknown) => {
// Do nothing
};
for (const callback of [undefined, onException]) {
Expand Down Expand Up @@ -480,7 +480,7 @@ describe('Utility', () => {
assert.strictEqual(attributes[SemanticAttributes.HTTP_ROUTE], undefined)
});
});

describe('headers to span attributes capture', () => {
let span: Span;

Expand Down