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: improve http span name #3603

Merged
merged 6 commits into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* feat: improve http span name [#FixMe](https://github.com/open-telemetry/opentelemetry-js/pull/FixMe) @Flarna
Flarna marked this conversation as resolved.
Show resolved Hide resolved
* feat: add HTTP_ROUTE attribute to http incoming metrics if present [#3581](https://github.com/open-telemetry/opentelemetry-js/pull/3581) @hermogenes

### :bug: (Bug Fix)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import {
} from '@opentelemetry/instrumentation';
import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core';
import { errorMonitor } from 'events';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';

/**
* Http instrumentation instrumentation for Opentelemetry
Expand Down Expand Up @@ -491,11 +492,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
utils.getIncomingRequestMetricAttributes(spanAttributes);

const ctx = propagation.extract(ROOT_CONTEXT, headers);
const span = instrumentation._startHttpSpan(
`${component.toLocaleUpperCase()} ${method}`,
spanOptions,
ctx
);
const span = instrumentation._startHttpSpan(method, spanOptions, ctx);
const rpcMetadata: RPCMetadata = {
type: RPCType.HTTP,
span,
Expand Down Expand Up @@ -624,7 +621,6 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
return original.apply(this, [optionsParsed, ...args]);
}

const operationName = `${component.toUpperCase()} ${method}`;
const { hostname, port } = utils.extractHostnameAndPort(optionsParsed);

const attributes = utils.getOutgoingRequestAttributes(optionsParsed, {
Expand All @@ -645,7 +641,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
kind: SpanKind.CLIENT,
attributes,
};
const span = instrumentation._startHttpSpan(operationName, spanOptions);
const span = instrumentation._startHttpSpan(method, spanOptions);

const parentContext = context.active();
const requestContext = trace.setSpan(parentContext, span);
Expand Down Expand Up @@ -720,6 +716,11 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
code: utils.parseResponseStatus(SpanKind.SERVER, response.statusCode),
});

const route = attributes[SemanticAttributes.HTTP_ROUTE];
if (route) {
span.updateName(`${request.method || 'GET'} ${route}`);
}
Comment on lines +716 to +719
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an entry for this new feature in the changelog as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

done


if (this._getConfig().applyCustomAttributesOnSpan) {
safeExecuteInTheMiddle(
() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ describe('HttpInstrumentation', () => {
if (request.url?.includes('/ignored')) {
provider.getTracer('test').startSpan('some-span').end();
}
if (request.url?.includes('/setroute')) {
const rpcData = getRPCMetadata(context.active());
assert.ok(rpcData != null);
assert.strictEqual(rpcData.type, RPCType.HTTP);
assert.strictEqual(rpcData.route, undefined);
rpcData.route = 'TheRoute';
}
response.end('Test Server Response');
});

Expand Down Expand Up @@ -339,6 +346,20 @@ describe('HttpInstrumentation', () => {
});
});

it('should respect HTTP_ROUTE', async () => {
await httpRequest.get(
`${protocol}://${hostname}:${serverPort}/setroute`
);
const span = memoryExporter.getFinishedSpans()[0];

assert.strictEqual(span.kind, SpanKind.SERVER);
assert.strictEqual(
span.attributes[SemanticAttributes.HTTP_ROUTE],
'TheRoute'
);
assert.strictEqual(span.name, 'GET TheRoute');
});

const httpErrorCodes = [
400, 401, 403, 404, 429, 501, 503, 504, 500, 505, 597,
];
Expand Down Expand Up @@ -404,7 +425,7 @@ describe('HttpInstrumentation', () => {

assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(spans.length, 2);
assert.strictEqual(reqSpan.name, 'HTTP GET');
assert.strictEqual(reqSpan.name, 'GET');
assert.strictEqual(
localSpan.spanContext().traceId,
reqSpan.spanContext().traceId
Expand Down Expand Up @@ -449,7 +470,7 @@ describe('HttpInstrumentation', () => {

assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(spans.length, 2);
assert.strictEqual(reqSpan.name, 'HTTP GET');
assert.strictEqual(reqSpan.name, 'GET');
assert.strictEqual(
localSpan.spanContext().traceId,
reqSpan.spanContext().traceId
Expand All @@ -474,7 +495,7 @@ describe('HttpInstrumentation', () => {
for (let i = 0; i < num; i++) {
await httpRequest.get(`${protocol}://${hostname}${testPath}`);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans[i].name, 'HTTP GET');
assert.strictEqual(spans[i].name, 'GET');
assert.strictEqual(
span.spanContext().traceId,
spans[i].spanContext().traceId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe('Packages', () => {
};

assert.strictEqual(spans.length, 1);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');

switch (name) {
case 'axios':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ describe('HttpsInstrumentation', () => {

assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(spans.length, 2);
assert.strictEqual(reqSpan.name, 'HTTPS GET');
assert.strictEqual(reqSpan.name, 'GET');
assert.strictEqual(
localSpan.spanContext().traceId,
reqSpan.spanContext().traceId
Expand Down Expand Up @@ -401,7 +401,7 @@ describe('HttpsInstrumentation', () => {

assert.ok(localSpan.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(spans.length, 2);
assert.strictEqual(reqSpan.name, 'HTTPS GET');
assert.strictEqual(reqSpan.name, 'GET');
assert.strictEqual(
localSpan.spanContext().traceId,
reqSpan.spanContext().traceId
Expand All @@ -426,7 +426,7 @@ describe('HttpsInstrumentation', () => {
for (let i = 0; i < num; i++) {
await httpsRequest.get(`${protocol}://${hostname}${testPath}`);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans[i].name, 'HTTPS GET');
assert.strictEqual(spans[i].name, 'GET');
assert.strictEqual(
span.spanContext().traceId,
spans[i].spanContext().traceId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ describe('Packages', () => {
};

assert.strictEqual(spans.length, 1);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');

switch (name) {
case 'axios':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ describe('HttpInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});

Expand All @@ -203,7 +203,7 @@ describe('HttpInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});

Expand Down Expand Up @@ -233,7 +233,7 @@ describe('HttpInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assert.strictEqual(result.reqHeaders['x-foo'], 'foo');
assert.strictEqual(
span.attributes[SemanticAttributes.HTTP_FLAVOR],
Expand Down Expand Up @@ -264,7 +264,7 @@ describe('HttpInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assert.strictEqual(span.attributes['span kind'], SpanKind.CLIENT);
assertSpan(span, SpanKind.CLIENT, validations);
});
Expand Down Expand Up @@ -292,7 +292,7 @@ describe('HttpInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});
for (const headers of [
Expand Down Expand Up @@ -347,7 +347,7 @@ describe('HttpInstrumentation Integration tests', () => {
const span = spans.find(s => s.kind === SpanKind.CLIENT);
assert.ok(span);
assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assert.ok(data);
assert.ok(validations.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY]);
assert.ok(validations.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY]);
Expand All @@ -362,7 +362,7 @@ describe('HttpInstrumentation Integration tests', () => {
const span = spans.find((s: any) => s.kind === SpanKind.SERVER);
assert.ok(span);
assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
});

it('should have correct spans even when request timeout', async () => {
Expand All @@ -381,7 +381,7 @@ describe('HttpInstrumentation Integration tests', () => {
spans = memoryExporter.getFinishedSpans();
const span = spans.find(s => s.kind === SpanKind.CLIENT);
assert.ok(span);
assert.strictEqual(span.name, 'HTTP GET');
assert.strictEqual(span.name, 'GET');
assert.strictEqual(
span.attributes[SemanticAttributes.HTTP_HOST],
`localhost:${mockServerPort}`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ describe('HttpsInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});

Expand All @@ -207,7 +207,7 @@ describe('HttpsInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});

Expand Down Expand Up @@ -237,7 +237,7 @@ describe('HttpsInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assert.strictEqual(result.reqHeaders['x-foo'], 'foo');
assert.strictEqual(
span.attributes[SemanticAttributes.HTTP_FLAVOR],
Expand Down Expand Up @@ -268,7 +268,7 @@ describe('HttpsInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assert.strictEqual(span.attributes['span kind'], SpanKind.CLIENT);
assertSpan(span, SpanKind.CLIENT, validations);
});
Expand Down Expand Up @@ -296,7 +296,7 @@ describe('HttpsInstrumentation Integration tests', () => {
};

assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assertSpan(span, SpanKind.CLIENT, validations);
});
for (const headers of [
Expand Down Expand Up @@ -351,7 +351,7 @@ describe('HttpsInstrumentation Integration tests', () => {
const span = spans.find(s => s.kind === SpanKind.CLIENT);
assert.ok(span);
assert.strictEqual(spans.length, 2);
assert.strictEqual(span.name, 'HTTPS GET');
assert.strictEqual(span.name, 'GET');
assert.ok(data);
assert.ok(validations.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY]);
assert.ok(validations.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,7 @@ export const assertSpan = (
assert.strictEqual(span.spanContext().traceId.length, 32);
assert.strictEqual(span.spanContext().spanId.length, 16);
assert.strictEqual(span.kind, kind);
assert.strictEqual(
span.name,
`${validations.component.toUpperCase()} ${validations.httpMethod}`
);
assert.strictEqual(span.name, validations.httpMethod);
assert.strictEqual(
span.attributes[AttributeNames.HTTP_ERROR_MESSAGE],
span.status.message
Expand Down