Skip to content

Commit

Permalink
feat: improve http span name (#3603)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Flarna
Flarna authored Feb 28, 2023
1 parent abfe059 commit 79b06a4
Show file tree
Hide file tree
Showing 9 changed files with 55 additions and 33 deletions.
3 changes: 3 additions & 0 deletions experimental/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ All notable changes to experimental packages in this project will be documented

### :boom: Breaking Change

* feat: remove HTTP/HTTPS prefix from span name [#3603](/~https://github.com/open-telemetry/opentelemetry-js/pull/3603) @Flarna

### :rocket: (Enhancement)

* feat: use HTTP_ROUTE in span name [#3603](/~https://github.com/open-telemetry/opentelemetry-js/pull/3603) @Flarna
* feat: add HTTP_ROUTE attribute to http incoming metrics if present [#3581](/~https://github.com/open-telemetry/opentelemetry-js/pull/3581) @hermogenes
* feat(sdk-node): install diag logger with OTEL_LOG_LEVEL [#3627](/~https://github.com/open-telemetry/opentelemetry-js/pull/3627) @legendecas

Expand Down
15 changes: 8 additions & 7 deletions experimental/packages/opentelemetry-instrumentation-http/src/http.ts
View file
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 @@ -489,11 +490,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 @@ -622,7 +619,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 @@ -643,7 +639,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 @@ -717,6 +713,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}`);
}

if (this._getConfig().applyCustomAttributesOnSpan) {
safeExecuteInTheMiddle(
() =>
Expand Down
27 changes: 24 additions & 3 deletions ...rimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
View file
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
2 changes: 1 addition & 1 deletion ...imental/packages/opentelemetry-instrumentation-http/test/functionals/http-package.test.ts
View file
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
6 changes: 3 additions & 3 deletions ...imental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts
View file
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
2 changes: 1 addition & 1 deletion ...mental/packages/opentelemetry-instrumentation-http/test/functionals/https-package.test.ts
View file
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
16 changes: 8 additions & 8 deletions ...imental/packages/opentelemetry-instrumentation-http/test/integrations/http-enable.test.ts
View file
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
12 changes: 6 additions & 6 deletions ...mental/packages/opentelemetry-instrumentation-http/test/integrations/https-enable.test.ts
View file
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
5 changes: 1 addition & 4 deletions experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts
View file
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

0 comments on commit 79b06a4

Please sign in to comment.