From 7baa493f50fd0a967d169ddfa2b38137b8a3b330 Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 4 Oct 2024 10:00:03 +0200 Subject: [PATCH 1/5] test(instr-http): remove usages of `new Span` (#5035) --- CHANGELOG.md | 1 + .../test/functionals/utils.test.ts | 120 +++++++++--------- 2 files changed, 64 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dc782cfe6..1e42a90f7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se [#4992](https://github.com/open-telemetry/opentelemetry-js/pull/4992) * refactor(sdk-metrics): replace `MetricsAttributes` with `Attributes` [#5021](https://github.com/open-telemetry/opentelemetry-js/pull/5021) @david-luna * refactor(instrumentation-http): replace `SpanAttributes` and `MetricsAttributes` with `Attributes` [#5023](https://github.com/open-telemetry/opentelemetry-js/pull/5023) @david-luna +* test(instrumentation-http): remove usages of `new Span` in tests [#5035](https://github.com/open-telemetry/opentelemetry-js/pull/5035) @david-luna ## 1.26.0 diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts index c731a296d9..c091529c9f 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts @@ -16,12 +16,10 @@ import { Attributes, SpanStatusCode, - ROOT_CONTEXT, SpanKind, - TraceFlags, context, + Span, } from '@opentelemetry/api'; -import { BasicTracerProvider, Span } from '@opentelemetry/sdk-trace-base'; import { SEMATTRS_HTTP_REQUEST_CONTENT_LENGTH, SEMATTRS_HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED, @@ -257,26 +255,28 @@ describe('Utility', () => { describe('setSpanWithError()', () => { it('should have error attributes', () => { const errorMessage = 'test error'; - const span = new Span( - new BasicTracerProvider().getTracer('default'), - ROOT_CONTEXT, - 'test', - { spanId: '', traceId: '', traceFlags: TraceFlags.SAMPLED }, - SpanKind.INTERNAL - ); - utils.setSpanWithError( - span, - new Error(errorMessage), - SemconvStability.OLD - ); - const attributes = span.attributes; - assert.strictEqual( - attributes[AttributeNames.HTTP_ERROR_MESSAGE], - errorMessage - ); - assert.strictEqual(span.events.length, 1); - assert.strictEqual(span.events[0].name, 'exception'); - assert.ok(attributes[AttributeNames.HTTP_ERROR_NAME]); + const error = new Error(errorMessage); + const span = { + setAttribute: () => undefined, + setStatus: () => undefined, + recordException: () => undefined, + } as unknown as Span; + const mock = sinon.mock(span); + + mock + .expects('setAttribute') + .calledWithExactly(AttributeNames.HTTP_ERROR_NAME, 'error'); + mock + .expects('setAttribute') + .calledWithExactly(AttributeNames.HTTP_ERROR_MESSAGE, errorMessage); + mock.expects('setStatus').calledWithExactly({ + code: SpanStatusCode.ERROR, + message: errorMessage, + }); + mock.expects('recordException').calledWithExactly(error); + + utils.setSpanWithError(span, error, SemconvStability.OLD); + mock.verify(); }); }); @@ -537,40 +537,51 @@ describe('Utility', () => { describe('headers to span attributes capture', () => { let span: Span; + let mock: sinon.SinonMock; beforeEach(() => { - span = new Span( - new BasicTracerProvider().getTracer('default'), - ROOT_CONTEXT, - 'test', - { spanId: '', traceId: '', traceFlags: TraceFlags.SAMPLED }, - SpanKind.INTERNAL - ); + span = { + setAttribute: () => undefined, + } as unknown as Span; + mock = sinon.mock(span); }); it('should set attributes for request and response keys', () => { + mock + .expects('setAttribute') + .calledWithExactly('http.request.header.origin', ['localhost']); + mock + .expects('setAttribute') + .calledWithExactly('http.response.header.cookie', ['token=123']); + utils.headerCapture('request', ['Origin'])(span, () => 'localhost'); utils.headerCapture('response', ['Cookie'])(span, () => 'token=123'); - assert.deepStrictEqual(span.attributes['http.request.header.origin'], [ - 'localhost', - ]); - assert.deepStrictEqual(span.attributes['http.response.header.cookie'], [ - 'token=123', - ]); + mock.verify(); }); it('should set attributes for multiple values', () => { + mock + .expects('setAttribute') + .calledWithExactly('http.request.header.origin', [ + 'localhost', + 'www.example.com', + ]); + utils.headerCapture('request', ['Origin'])(span, () => [ 'localhost', 'www.example.com', ]); - assert.deepStrictEqual(span.attributes['http.request.header.origin'], [ - 'localhost', - 'www.example.com', - ]); + mock.verify(); }); it('sets attributes for multiple headers', () => { + mock + .expects('setAttribute') + .calledWithExactly('http.request.header.origin', ['localhost']); + mock + .expects('setAttribute') + .calledWithExactly('http.request.header.foo', [42]); + utils.headerCapture('request', ['Origin', 'Foo'])(span, header => { if (header === 'origin') { return 'localhost'; @@ -582,22 +593,24 @@ describe('Utility', () => { return undefined; }); - - assert.deepStrictEqual(span.attributes['http.request.header.origin'], [ - 'localhost', - ]); - assert.deepStrictEqual(span.attributes['http.request.header.foo'], [42]); + mock.verify(); }); it('should normalize header names', () => { + mock + .expects('setAttribute') + .calledWithExactly('http.request.header.x_forwarded_for', ['foo']); + utils.headerCapture('request', ['X-Forwarded-For'])(span, () => 'foo'); - assert.deepStrictEqual( - span.attributes['http.request.header.x_forwarded_for'], - ['foo'] - ); + mock.verify(); }); it('ignores non-existent headers', () => { + mock + .expects('setAttribute') + .once() + .calledWithExactly('http.request.header.origin', ['localhost']); + utils.headerCapture('request', ['Origin', 'Accept'])(span, header => { if (header === 'origin') { return 'localhost'; @@ -605,14 +618,7 @@ describe('Utility', () => { return undefined; }); - - assert.deepStrictEqual(span.attributes['http.request.header.origin'], [ - 'localhost', - ]); - assert.deepStrictEqual( - span.attributes['http.request.header.accept'], - undefined - ); + mock.verify(); }); }); From 776993f0fcefd150d1215042b5515d99a9f247b1 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 4 Oct 2024 04:35:04 -0400 Subject: [PATCH 2/5] feat(instrumentation-http): emit semconv 1.27 metrics (#5026) --- experimental/CHANGELOG.md | 8 +- .../README.md | 27 +- .../src/http.ts | 231 +++++++++-- .../test/functionals/http-metrics.test.ts | 387 ++++++++++++++---- 4 files changed, 526 insertions(+), 127 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 49eb60e237..37252e92cc 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -10,10 +10,10 @@ All notable changes to experimental packages in this project will be documented ### :rocket: (Enhancement) * feat(api-logs): Add delegating no-op logger provider [#4861](https://github.com/open-telemetry/opentelemetry-js/pull/4861) @hectorhdzg -* feat(instrumentation-http): Add support for [Semantic Conventions 1.27+](https://github.com/open-telemetry/semantic-conventions/releases/tag/v1.27.0) [#4940](https://github.com/open-telemetry/opentelemetry-js/pull/4940) [#4978](https://github.com/open-telemetry/opentelemetry-js/pull/4978) @dyladan - * Applies to both client and server spans - * Generate spans compliant with Semantic Conventions 1.27+ when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http` or `http/dup` - * Generate spans backwards compatible with previous attributes when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http/dup` or DOES NOT contain `http` +* feat(instrumentation-http): Add support for [Semantic Conventions 1.27+](https://github.com/open-telemetry/semantic-conventions/releases/tag/v1.27.0) [#4940](https://github.com/open-telemetry/opentelemetry-js/pull/4940) [#4978](https://github.com/open-telemetry/opentelemetry-js/pull/4978) [#5026](https://github.com/open-telemetry/opentelemetry-js/pull/5026) @dyladan + * Applies to client and server spans and metrics + * Generate spans and metrics compliant with Semantic Conventions 1.27+ when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http` or `http/dup` + * Generate spans and metrics backwards compatible with previous attributes when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http/dup` or DOES NOT contain `http` ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-instrumentation-http/README.md b/experimental/packages/opentelemetry-instrumentation-http/README.md index 456d5b3473..030e8f327b 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/README.md +++ b/experimental/packages/opentelemetry-instrumentation-http/README.md @@ -76,8 +76,6 @@ The following options are deprecated: ## Semantic Conventions -### Client and Server Spans - Prior to version `0.54`, this instrumentation created spans targeting an experimental semantic convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md). This package is capable of emitting both Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md) and [Version 1.27.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-spans.md). @@ -86,21 +84,19 @@ The values `http` and `http/dup` control this instrumentation. See details for the behavior of each of these values below. If neither `http` or `http/dup` is included in `OTEL_SEMCONV_STABILITY_OPT_IN`, the old experimental semantic conventions will be used by default. -#### Stable Semantic Conventions 1.27 +### Stable Semantic Conventions 1.27 Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http` OR `http/dup`. This is the recommended configuration, and will soon become the default behavior. -Follow all requirements and recommendations of HTTP Client and Server Span Semantic Conventions [Version 1.27.0](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-spans.md), including all required and recommended attributes. - -#### Legacy Behavior (default) +Follow all requirements and recommendations of HTTP Client and Server Semantic Conventions Version 1.27.0 for [spans](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-spans.md) and [metrics](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-metrics.md), including all required and recommended attributes. -Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http/dup` or DOES NOT CONTAIN `http`. -This is the current default behavior. +Metrics Exported: -Create HTTP client spans which implement Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md). +- [`http.server.request.duration`](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-metrics.md#metric-httpserverrequestduration) +- [`http.client.request.duration`](https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/http/http-metrics.md#metric-httpclientrequestduration) -#### Upgrading Semantic Conventions +### Upgrading Semantic Conventions When upgrading to the new semantic conventions, it is recommended to do so in the following order: @@ -111,9 +107,16 @@ When upgrading to the new semantic conventions, it is recommended to do so in th This will cause both the old and new semantic conventions to be emitted during the transition period. -### Server Spans +### Legacy Behavior (default) + +Enabled when `OTEL_SEMCONV_STABILITY_OPT_IN` contains `http/dup` or DOES NOT CONTAIN `http`. +This is the current default behavior. + +Create HTTP client spans which implement Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md). + +#### Server Spans (legacy) -This package uses `@opentelemetry/semantic-conventions` version `1.22+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md). +When `OTEL_SEMCONV_STABILITY_OPT_IN` is not set or includes `http/dup`, this module implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md). Attributes collected: diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index 456b8d08e0..0d0a63920b 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -62,7 +62,18 @@ import { getEnv, } from '@opentelemetry/core'; import { errorMonitor } from 'events'; -import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; +import { + ATTR_HTTP_REQUEST_METHOD, + ATTR_HTTP_RESPONSE_STATUS_CODE, + ATTR_HTTP_ROUTE, + ATTR_NETWORK_PROTOCOL_VERSION, + ATTR_SERVER_ADDRESS, + ATTR_SERVER_PORT, + ATTR_URL_SCHEME, + METRIC_HTTP_CLIENT_REQUEST_DURATION, + METRIC_HTTP_SERVER_REQUEST_DURATION, + SEMATTRS_HTTP_ROUTE, +} from '@opentelemetry/semantic-conventions'; import { extractHostnameAndPort, getIncomingRequestAttributes, @@ -88,8 +99,10 @@ export class HttpInstrumentation extends InstrumentationBase = new WeakSet(); private _headerCapture; - private _httpServerDurationHistogram!: Histogram; - private _httpClientDurationHistogram!: Histogram; + private _oldHttpServerDurationHistogram!: Histogram; + private _stableHttpServerDurationHistogram!: Histogram; + private _oldHttpClientDurationHistogram!: Histogram; + private _stableHttpClientDurationHistogram!: Histogram; private _semconvStability = SemconvStability.OLD; @@ -109,7 +122,7 @@ export class HttpInstrumentation extends InstrumentationBase { this._diag.debug('outgoingRequest on request error()', error); @@ -458,7 +558,13 @@ export class HttpInstrumentation extends InstrumentationBase { + let contextManager: ContextManager; + beforeEach(() => { + contextManager = new AsyncHooksContextManager().enable(); + context.setGlobalContextManager(contextManager); + instrumentation['_updateMetricInstruments'](); metricsMemoryExporter.reset(); }); before(() => { + instrumentation.setConfig({}); instrumentation.enable(); server = http.createServer((request, response) => { + 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'); }); server.listen(serverPort); @@ -74,87 +95,289 @@ describe('metrics', () => { instrumentation.disable(); }); - it('should add server/client duration metrics', async () => { - const requestCount = 3; - for (let i = 0; i < requestCount; i++) { - await httpRequest.get( - `${protocol}://${hostname}:${serverPort}${pathname}` - ); - } - await metricReader.collectAndExport(); - const resourceMetrics = metricsMemoryExporter.getMetrics(); - const scopeMetrics = resourceMetrics[0].scopeMetrics; - assert.strictEqual(scopeMetrics.length, 1, 'scopeMetrics count'); - const metrics = scopeMetrics[0].metrics; - assert.strictEqual(metrics.length, 2, 'metrics count'); - assert.strictEqual(metrics[0].dataPointType, DataPointType.HISTOGRAM); - assert.strictEqual( - metrics[0].descriptor.description, - 'Measures the duration of inbound HTTP requests.' - ); - assert.strictEqual(metrics[0].descriptor.name, 'http.server.duration'); - assert.strictEqual(metrics[0].descriptor.unit, 'ms'); - assert.strictEqual(metrics[0].dataPoints.length, 1); - assert.strictEqual( - (metrics[0].dataPoints[0].value as any).count, - requestCount - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_SCHEME], - 'http' - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], - 'GET' - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], - '1.1' - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_NAME], - 'localhost' - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], - 200 - ); - assert.strictEqual( - metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_PORT], - 22346 - ); - - assert.strictEqual(metrics[1].dataPointType, DataPointType.HISTOGRAM); - assert.strictEqual( - metrics[1].descriptor.description, - 'Measures the duration of outbound HTTP requests.' - ); - assert.strictEqual(metrics[1].descriptor.name, 'http.client.duration'); - assert.strictEqual(metrics[1].descriptor.unit, 'ms'); - assert.strictEqual(metrics[1].dataPoints.length, 1); - assert.strictEqual( - (metrics[1].dataPoints[0].value as any).count, - requestCount - ); - assert.strictEqual( - metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], - 'GET' - ); - assert.strictEqual( - metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_NAME], - 'localhost' - ); - assert.strictEqual( - metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_PORT], - 22346 - ); - assert.strictEqual( - metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], - 200 - ); - assert.strictEqual( - metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], - '1.1' - ); + describe('with no stability set', () => { + it('should add server/client duration metrics', async () => { + const requestCount = 3; + for (let i = 0; i < requestCount; i++) { + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${pathname}` + ); + } + await metricReader.collectAndExport(); + const resourceMetrics = metricsMemoryExporter.getMetrics(); + const scopeMetrics = resourceMetrics[0].scopeMetrics; + assert.strictEqual(scopeMetrics.length, 1, 'scopeMetrics count'); + const metrics = scopeMetrics[0].metrics; + assert.strictEqual(metrics.length, 2, 'metrics count'); + assert.strictEqual(metrics[0].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[0].descriptor.description, + 'Measures the duration of inbound HTTP requests.' + ); + assert.strictEqual(metrics[0].descriptor.name, 'http.server.duration'); + assert.strictEqual(metrics[0].descriptor.unit, 'ms'); + assert.strictEqual(metrics[0].dataPoints.length, 1); + assert.strictEqual( + (metrics[0].dataPoints[0].value as any).count, + requestCount + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_SCHEME], + 'http' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], + 'GET' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], + '1.1' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_NAME], + 'localhost' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], + 200 + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_PORT], + 22346 + ); + + assert.strictEqual(metrics[1].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[1].descriptor.description, + 'Measures the duration of outbound HTTP requests.' + ); + assert.strictEqual(metrics[1].descriptor.name, 'http.client.duration'); + assert.strictEqual(metrics[1].descriptor.unit, 'ms'); + assert.strictEqual(metrics[1].dataPoints.length, 1); + assert.strictEqual( + (metrics[1].dataPoints[0].value as any).count, + requestCount + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], + 'GET' + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_NAME], + 'localhost' + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_PORT], + 22346 + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], + 200 + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], + '1.1' + ); + }); + }); + + describe('with no semconv stability set to stable', () => { + before(() => { + instrumentation['_semconvStability'] = SemconvStability.STABLE; + }); + + it('should add server/client duration metrics', async () => { + const requestCount = 3; + for (let i = 0; i < requestCount; i++) { + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${pathname}` + ); + } + await metricReader.collectAndExport(); + const resourceMetrics = metricsMemoryExporter.getMetrics(); + const scopeMetrics = resourceMetrics[0].scopeMetrics; + assert.strictEqual(scopeMetrics.length, 1, 'scopeMetrics count'); + const metrics = scopeMetrics[0].metrics; + assert.strictEqual(metrics.length, 2, 'metrics count'); + assert.strictEqual(metrics[0].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[0].descriptor.description, + 'Duration of HTTP server requests.' + ); + assert.strictEqual( + metrics[0].descriptor.name, + 'http.server.request.duration' + ); + assert.strictEqual(metrics[0].descriptor.unit, 's'); + assert.strictEqual(metrics[0].dataPoints.length, 1); + assert.strictEqual( + (metrics[0].dataPoints[0].value as any).count, + requestCount + ); + assert.deepStrictEqual(metrics[0].dataPoints[0].attributes, { + [ATTR_HTTP_REQUEST_METHOD]: 'GET', + [ATTR_URL_SCHEME]: 'http', + [ATTR_NETWORK_PROTOCOL_VERSION]: '1.1', + }); + + assert.strictEqual(metrics[1].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[1].descriptor.description, + 'Duration of HTTP client requests.' + ); + assert.strictEqual( + metrics[1].descriptor.name, + 'http.client.request.duration' + ); + assert.strictEqual(metrics[1].descriptor.unit, 's'); + assert.strictEqual(metrics[1].dataPoints.length, 1); + assert.strictEqual( + (metrics[1].dataPoints[0].value as any).count, + requestCount + ); + + assert.deepStrictEqual(metrics[1].dataPoints[0].attributes, { + [ATTR_HTTP_REQUEST_METHOD]: 'GET', + [ATTR_SERVER_ADDRESS]: 'localhost', + [ATTR_SERVER_PORT]: 22346, + }); + }); + }); + + describe('with no semconv stability set to duplicate', () => { + before(() => { + instrumentation['_semconvStability'] = SemconvStability.DUPLICATE; + }); + + it('should add server/client duration metrics', async () => { + const requestCount = 3; + for (let i = 0; i < requestCount; i++) { + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${pathname}` + ); + } + await metricReader.collectAndExport(); + const resourceMetrics = metricsMemoryExporter.getMetrics(); + const scopeMetrics = resourceMetrics[0].scopeMetrics; + assert.strictEqual(scopeMetrics.length, 1, 'scopeMetrics count'); + const metrics = scopeMetrics[0].metrics; + assert.strictEqual(metrics.length, 4, 'metrics count'); + + // old metrics + assert.strictEqual(metrics[0].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[0].descriptor.description, + 'Measures the duration of inbound HTTP requests.' + ); + assert.strictEqual(metrics[0].descriptor.name, 'http.server.duration'); + assert.strictEqual(metrics[0].descriptor.unit, 'ms'); + assert.strictEqual(metrics[0].dataPoints.length, 1); + assert.strictEqual( + (metrics[0].dataPoints[0].value as any).count, + requestCount + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_SCHEME], + 'http' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], + 'GET' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], + '1.1' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_NAME], + 'localhost' + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], + 200 + ); + assert.strictEqual( + metrics[0].dataPoints[0].attributes[SEMATTRS_NET_HOST_PORT], + 22346 + ); + + assert.strictEqual(metrics[1].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[1].descriptor.description, + 'Measures the duration of outbound HTTP requests.' + ); + assert.strictEqual(metrics[1].descriptor.name, 'http.client.duration'); + assert.strictEqual(metrics[1].descriptor.unit, 'ms'); + assert.strictEqual(metrics[1].dataPoints.length, 1); + assert.strictEqual( + (metrics[1].dataPoints[0].value as any).count, + requestCount + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_METHOD], + 'GET' + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_NAME], + 'localhost' + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_NET_PEER_PORT], + 22346 + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_STATUS_CODE], + 200 + ); + assert.strictEqual( + metrics[1].dataPoints[0].attributes[SEMATTRS_HTTP_FLAVOR], + '1.1' + ); + + // Stable metrics + assert.strictEqual(metrics[2].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[2].descriptor.description, + 'Duration of HTTP server requests.' + ); + assert.strictEqual( + metrics[2].descriptor.name, + 'http.server.request.duration' + ); + assert.strictEqual(metrics[2].descriptor.unit, 's'); + assert.strictEqual(metrics[2].dataPoints.length, 1); + assert.strictEqual( + (metrics[2].dataPoints[0].value as any).count, + requestCount + ); + assert.deepStrictEqual(metrics[2].dataPoints[0].attributes, { + [ATTR_HTTP_REQUEST_METHOD]: 'GET', + [ATTR_URL_SCHEME]: 'http', + [ATTR_NETWORK_PROTOCOL_VERSION]: '1.1', + [ATTR_HTTP_ROUTE]: 'TheRoute', + }); + + assert.strictEqual(metrics[3].dataPointType, DataPointType.HISTOGRAM); + assert.strictEqual( + metrics[3].descriptor.description, + 'Duration of HTTP client requests.' + ); + assert.strictEqual( + metrics[3].descriptor.name, + 'http.client.request.duration' + ); + assert.strictEqual(metrics[3].descriptor.unit, 's'); + assert.strictEqual(metrics[3].dataPoints.length, 1); + assert.strictEqual( + (metrics[3].dataPoints[0].value as any).count, + requestCount + ); + + assert.deepStrictEqual(metrics[3].dataPoints[0].attributes, { + [ATTR_HTTP_REQUEST_METHOD]: 'GET', + [ATTR_SERVER_ADDRESS]: 'localhost', + [ATTR_SERVER_PORT]: 22346, + }); + }); }); }); From 4947c2d6a4d33343194879c3b3eb18040eb9683b Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 4 Oct 2024 10:42:35 +0200 Subject: [PATCH 3/5] chore(exporter-zipkin): remove usages of Span constructor (#5030) Co-authored-by: Marc Pichler --- CHANGELOG.md | 1 + .../test/common/transform.test.ts | 229 +++++++++--------- 2 files changed, 109 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e42a90f7c..7ad9ff304d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se [#4992](https://github.com/open-telemetry/opentelemetry-js/pull/4992) * refactor(sdk-metrics): replace `MetricsAttributes` with `Attributes` [#5021](https://github.com/open-telemetry/opentelemetry-js/pull/5021) @david-luna * refactor(instrumentation-http): replace `SpanAttributes` and `MetricsAttributes` with `Attributes` [#5023](https://github.com/open-telemetry/opentelemetry-js/pull/5023) @david-luna +* chore(exporter-zipkin): remove usages of Span constructor [#5030](https://github.com/open-telemetry/opentelemetry-js/pull/5030) @david-luna * test(instrumentation-http): remove usages of `new Span` in tests [#5035](https://github.com/open-telemetry/opentelemetry-js/pull/5035) @david-luna ## 1.26.0 diff --git a/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts b/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts index 223257c45a..0c78dbbcf8 100644 --- a/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts +++ b/packages/opentelemetry-exporter-zipkin/test/common/transform.test.ts @@ -16,17 +16,16 @@ import * as api from '@opentelemetry/api'; import { + hrTime, hrTimeDuration, hrTimeToMicroseconds, + millisToHrTime, VERSION, } from '@opentelemetry/core'; -import { Resource } from '@opentelemetry/resources'; -import { BasicTracerProvider, Span } from '@opentelemetry/sdk-trace-base'; +import { IResource } from '@opentelemetry/resources'; +import { ReadableSpan } from '@opentelemetry/sdk-trace-base'; import * as assert from 'assert'; -import { - SEMRESATTRS_SERVICE_NAME, - SEMRESATTRS_TELEMETRY_SDK_LANGUAGE, -} from '@opentelemetry/semantic-conventions'; +import { SEMRESATTRS_SERVICE_NAME } from '@opentelemetry/semantic-conventions'; import { defaultStatusCodeTagName, defaultStatusErrorTagName, @@ -35,43 +34,69 @@ import { _toZipkinTags, } from '../../src/transform'; import * as zipkinTypes from '../../src/types'; -const tracer = new BasicTracerProvider({ - resource: Resource.default().merge( - new Resource({ - [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', - cost: '112.12', - service: 'ui', - version: '1', - }) - ), -}).getTracer('default'); - -const language = tracer.resource.attributes[SEMRESATTRS_TELEMETRY_SDK_LANGUAGE]; +const resource = { + attributes: { + [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', + cost: '112.12', + service: 'ui', + version: '1', + 'telemetry.sdk.language': 'nodejs', + 'telemetry.sdk.name': 'opentelemetry', + 'telemetry.sdk.version': VERSION, + }, +} as unknown as IResource; const parentId = '5c1c63257de34c67'; const spanContext: api.SpanContext = { traceId: 'd4cda95b652f4a1592b449d5929fda1b', spanId: '6e0c63257de34c92', traceFlags: api.TraceFlags.SAMPLED, }; +const currentTime = Date.now(); +const durationMs = 10; +const startTime = hrTime(currentTime - durationMs); +const endTime = hrTime(currentTime); +const duration = millisToHrTime(durationMs); + +function getSpan(options: Partial): ReadableSpan { + const span = { + name: options.name || 'my-span', + kind: typeof options.kind === 'number' ? options.kind : api.SpanKind.SERVER, + startTime: options.startTime || startTime, + endTime: options.endTime || endTime, + duration: options.duration || duration, + spanContext: () => spanContext, + parentSpanId: options.parentSpanId || parentId, + attributes: options.attributes || {}, + events: options.events || [], + status: options.status || { code: api.SpanStatusCode.UNSET }, + resource, + } as ReadableSpan; + + // Expicit `undefined` properties in options will be removed from the + // result span. + Object.keys(options).forEach(k => { + if (options[k as keyof ReadableSpan] === undefined) { + delete span[k as keyof ReadableSpan]; + } + }); + + return span; +} describe('transform', () => { describe('toZipkinSpan', () => { it('should convert an OpenTelemetry span to a Zipkin span', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId - ); - span.setAttributes({ - key1: 'value1', - key2: 'value2', + const span = getSpan({ + attributes: { key1: 'value1', key2: 'value2' }, + events: [ + { + name: 'my-event', + time: hrTime(Date.now() + 5), + attributes: { key3: 'value 3' }, + }, + ], }); - span.addEvent('my-event', { key3: 'value3' }); - span.end(); const zipkinSpan = toZipkinSpan( span, @@ -103,7 +128,7 @@ describe('transform', () => { cost: '112.12', service: 'ui', version: '1', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, }, @@ -112,14 +137,9 @@ describe('transform', () => { }); }); it("should skip parentSpanId if doesn't exist", () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER - ); - span.end(); + const span = getSpan({ + parentSpanId: undefined, + }); const zipkinSpan = toZipkinSpan( span, @@ -144,7 +164,7 @@ describe('transform', () => { cost: '112.12', service: 'ui', version: '1', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, }, @@ -163,15 +183,10 @@ describe('transform', () => { it(`should map OpenTelemetry SpanKind ${ api.SpanKind[item.ot] } to Zipkin ${item.zipkin}`, () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - item.ot - ); - span.end(); - + const span = getSpan({ + kind: item.ot, + parentSpanId: undefined, + }); const zipkinSpan = toZipkinSpan( span, 'my-service', @@ -195,7 +210,7 @@ describe('transform', () => { cost: '112.12', service: 'ui', version: '1', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, }, @@ -208,17 +223,12 @@ describe('transform', () => { describe('_toZipkinTags', () => { it('should convert OpenTelemetry attributes to Zipkin tags', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId - ); - span.setAttributes({ - key1: 'value1', - key2: 'value2', + const span = getSpan({ + parentSpanId: undefined, + attributes: { + key1: 'value1', + key2: 'value2', + }, }); const tags: zipkinTypes.Tags = _toZipkinTags( span, @@ -230,7 +240,7 @@ describe('transform', () => { key1: 'value1', key2: 'value2', [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, cost: '112.12', @@ -239,21 +249,14 @@ describe('transform', () => { }); }); it('should map OpenTelemetry constructor attributes to a Zipkin tag', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId, - [], - undefined, - undefined, - { + const span = getSpan({ + parentSpanId: undefined, + attributes: { key1: 'value1', key2: 'value2', - } - ); + }, + }); + const tags: zipkinTypes.Tags = _toZipkinTags( span, defaultStatusCodeTagName, @@ -264,7 +267,7 @@ describe('transform', () => { key1: 'value1', key2: 'value2', [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, cost: '112.12', @@ -273,21 +276,13 @@ describe('transform', () => { }); }); it('should map OpenTelemetry SpanStatus.code to a Zipkin tag', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId - ); - const status: api.SpanStatus = { - code: api.SpanStatusCode.ERROR, - }; - span.setStatus(status); - span.setAttributes({ - key1: 'value1', - key2: 'value2', + const span = getSpan({ + parentSpanId: undefined, + attributes: { + key1: 'value1', + key2: 'value2', + }, + status: { code: api.SpanStatusCode.ERROR }, }); const tags: zipkinTypes.Tags = _toZipkinTags( span, @@ -300,7 +295,7 @@ describe('transform', () => { key2: 'value2', [defaultStatusCodeTagName]: 'ERROR', [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, cost: '112.12', @@ -309,22 +304,13 @@ describe('transform', () => { }); }); it('should map OpenTelemetry SpanStatus.message to a Zipkin tag', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId - ); - const status: api.SpanStatus = { - code: api.SpanStatusCode.ERROR, - message: 'my-message', - }; - span.setStatus(status); - span.setAttributes({ - key1: 'value1', - key2: 'value2', + const span = getSpan({ + parentSpanId: undefined, + attributes: { + key1: 'value1', + key2: 'value2', + }, + status: { code: api.SpanStatusCode.ERROR, message: 'my-message' }, }); const tags: zipkinTypes.Tags = _toZipkinTags( span, @@ -336,9 +322,9 @@ describe('transform', () => { key1: 'value1', key2: 'value2', [defaultStatusCodeTagName]: 'ERROR', - [defaultStatusErrorTagName]: status.message, + [defaultStatusErrorTagName]: 'my-message', [SEMRESATTRS_SERVICE_NAME]: 'zipkin-test', - 'telemetry.sdk.language': language, + 'telemetry.sdk.language': 'nodejs', 'telemetry.sdk.name': 'opentelemetry', 'telemetry.sdk.version': VERSION, cost: '112.12', @@ -350,16 +336,17 @@ describe('transform', () => { describe('_toZipkinAnnotations', () => { it('should convert OpenTelemetry events to Zipkin annotations', () => { - const span = new Span( - tracer, - api.ROOT_CONTEXT, - 'my-span', - spanContext, - api.SpanKind.SERVER, - parentId - ); - span.addEvent('my-event1'); - span.addEvent('my-event2', { key1: 'value1' }); + const span = getSpan({ + parentSpanId: undefined, + events: [ + { name: 'my-event1', time: hrTime(Date.now()) }, + { + name: 'my-event2', + time: hrTime(Date.now()), + attributes: { key1: 'value1' }, + }, + ], + }); const annotations = _toZipkinAnnotations(span.events); assert.deepStrictEqual(annotations, [ From 6ccd4df33a47eb9b3e56a675a4f217b4844e18cd Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 7 Oct 2024 22:02:54 +0200 Subject: [PATCH 4/5] docs: use npm ci in CONTRIBUTING.md (#5040) --- CONTRIBUTING.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 516e67d524..374f0470d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -29,9 +29,9 @@ detailed instructions, see [development](#development) below. ```sh git clone https://github.com/open-telemetry/opentelemetry-js.git cd opentelemetry-js -npm install +npm ci npm run compile -npm test +npm run test ``` ## Pull Request Merge Guidelines @@ -164,7 +164,7 @@ Most of the commands needed for development are accessed as [npm scripts](https: This will install all dependencies for the root project and all modules managed by `npm workspaces`. ```sh -npm install +npm ci ``` ### Compile modules @@ -293,10 +293,10 @@ export const _globalThis = typeof globalThis === 'object' ? globalThis : global; /// packages/opentelemetry-core/src/platform/browser/globalThis.ts export const _globalThis: typeof globalThis = typeof globalThis === 'object' ? globalThis : - typeof self === 'object' ? self : - typeof window === 'object' ? window : - typeof global === 'object' ? global : - {} as typeof globalThis; + typeof self === 'object' ? self : + typeof window === 'object' ? window : + typeof global === 'object' ? global : + {} as typeof globalThis; ``` Even though the implementation may differ, the exported names must be aligned. From 859c0ef5a0746afe9d4b3ef72505d20fab4448e0 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Wed, 9 Oct 2024 10:04:17 +0200 Subject: [PATCH 5/5] ci: use codecov token when uploading reports (#5053) --- .github/workflows/unit-test.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml index 11bf4a9c7a..55db405284 100644 --- a/.github/workflows/unit-test.yml +++ b/.github/workflows/unit-test.yml @@ -48,6 +48,8 @@ jobs: run: npm run test - name: Report Coverage uses: codecov/codecov-action@v4 + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} with: verbose: true node-windows-tests: @@ -102,6 +104,8 @@ jobs: run: npm run test:browser - name: Report Coverage uses: codecov/codecov-action@v4 + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} with: verbose: true webworker-tests: @@ -128,6 +132,8 @@ jobs: run: npm run test:webworker - name: Report Coverage uses: codecov/codecov-action@v4 + env: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} with: verbose: true api-eol-node-test: