diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fc77f47ad67..a2215d32696 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -105,7 +105,6 @@ Please also see [GitHub workflow](https://github.com/open-telemetry/community/bl - [TypeScript](https://www.typescriptlang.org/) - [lerna](https://github.com/lerna/lerna) to manage dependencies, compilations, and links between packages. Most lerna commands should be run by calling the provided npm scripts. - [MochaJS](https://mochajs.org/) for tests -- [gts](https://github.com/google/gts) - [eslint](https://eslint.org/) Most of the commands needed for development are accessed as [npm scripts](https://docs.npmjs.com/cli/v6/using-npm/scripts). It is recommended that you use the provided npm scripts instead of using `lerna run` in most cases. @@ -167,9 +166,16 @@ cd packages/opentelemetry-module-name npm test ``` +To run the unit tests continuously in watch mode while developing, use: + +```sh +# Run test in watch mode +npm run tdd +``` + ### Linting -This project uses a combination of `gts` and `eslint`. Just like tests and compilation, linting can be done for all packages or only a single package. +This project uses `eslint` to lint source code. Just like tests and compilation, linting can be done for all packages or only a single package. ```sh # Lint all modules @@ -191,6 +197,19 @@ cd packages/opentelemetry-module-name npm run lint:fix ``` +### Generating docs + +We use [typedoc](https://www.npmjs.com/package/typedoc) to generate the api documentation. + +To generate the docs, use: + +```sh +# Generate docs in the root 'docs' directory +npm run docs +``` + +The document will be available under `docs` path. + ### Adding a package To add a new package, copy `packages/template` to your new package directory and modify the `package.json` file to reflect your desired package settings. If the package will not support browser, the `karma.conf` and `tsconifg.esm.json` files may be deleted. If the package will support es5 targets, the reference to `tsconfig.base.json` in `tsconfig.json` should be changed to `tsconfig.es5.json`. @@ -226,11 +245,37 @@ After adding the package, run `npm install` from the root of the project. This w If all of the above requirements are met and there are no unresolved discussions, a pull request may be merged by either a maintainer or an approver. -### Generating API documentation - -- `npm run docs` to generate API documentation. Generates the documentation in `packages/opentelemetry-api/docs/out` - ### Generating CHANGELOG documentation - Generate and export your [Github access token](https://docs.github.com/en/github/authenticating-to-github/creating-a-personal-access-token): `export GITHUB_AUTH=` - `npm run changelog` to generate CHANGELOG documentation in your terminal (see [RELEASING.md](RELEASING.md) for more details). + +### Platform conditional exports + +Universal packages are packages that can be used in both web browsers and +Node.js environment. These packages may be implemented on top of different +platform APIs to achieve the same goal. Like accessing the _global_ reference, +we have different preferred ways to do it: + +- In Node.js, we access the _global_ reference with `globalThis` or `global`: + +```js +/// packages/opentelemetry-core/src/platform/node/globalThis.ts +export const _globalThis = typeof globalThis === 'object' ? globalThis : global; +``` + +- In web browser, we access the _global_ reference with the following definition: + +```js +/// 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; +``` + +Even though the implementation may differ, the exported names must be aligned. +It can be confusing if exported names present in one environment but not in the +others. diff --git a/README.md b/README.md index 94c927622c9..ee56336d0c7 100644 --- a/README.md +++ b/README.md @@ -31,8 +31,6 @@ Contributing   •   - Development Guide -   •   Examples

diff --git a/doc/development-guide.md b/doc/development-guide.md deleted file mode 100644 index f4dd02a63e6..00000000000 --- a/doc/development-guide.md +++ /dev/null @@ -1,94 +0,0 @@ -# Development Guide - -Before contributing to this open source project, read our [CONTRIBUTING](../CONTRIBUTING.md). We gratefully welcome improvements to documentation as well as to code. - -The code base is a monorepo. We use [Lerna](https://lerna.js.org/) for managing inter-module dependencies, which makes it easier to develop coordinated changes between the modules. Instead of running lerna directly, the commands are wrapped with `npm`; - -## Requirements - -Since this project supports multiple Node versions, using a version -manager such as [nvm](https://github.com/nvm-sh/nvm) is recommended. - -To get started once you have Node installed, run: - -```sh -npm install -``` - -This will install all the necessary modules. - -## Testing - -### Unit Tests - -To run the all unit tests, use: - -```sh -npm run test -``` - -To run the unit tests continuously in watch mode while developing, use: - -```sh -npm run tdd -``` - -## Linting - -We use [gts](https://www.npmjs.com/package/gts) to make sure that new code is conform to our coding standards. - -Before raising a pull request, make sure there are no lint problems. - -To check the linter, use: - -```sh -npm run lint -``` - -To fix the linter, use: - -```sh -npm run lint:fix -``` - -## Docs - -We use [typedoc](https://www.npmjs.com/package/typedoc) to generate the api documentation. - -To generate the docs, use: - -```sh -npm run docs -``` - -The document will be available under `packages/opentelemetry-api/docs/out` path. - -## Platform conditional exports - -Universal packages are packages that can be used in both web browsers and -Node.js environment. These packages may be implemented on top of different -platform APIs to achieve the same goal. Like accessing the _global_ reference, -we have different preferred ways to do it: - -- In Node.js, we access the _global_ reference with `globalThis` or `global`: - -```js -/// packages/opentelemetry-core/src/platform/node/globalThis.ts -export const _globalThis = typeof globalThis === 'object' ? globalThis : global; -``` - -- In web browser, we access the _global_ reference with the following definition: - -```js -/// 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; -``` - -Even though the implementation may differ, the exported names must be aligned. -It can be confusing if exported names present in one environment but not in the -others. diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 0f2483a7db5..a0315f553e0 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -13,6 +13,7 @@ All notable changes to experimental packages in this project will be documented ### :rocket: (Enhancement) +* feat(metrics-api): use common attributes definitions #3038 @legendecas * feat(otlp-proto): pre-compile proto files [#3098](https://github.com/open-telemetry/opentelemetry-js/pull/3098) @legendecas ### :bug: (Bug Fix) @@ -20,6 +21,8 @@ All notable changes to experimental packages in this project will be documented * fix(histogram): fix maximum when only values < -1 are provided [#3086](https://github.com/open-telemetry/opentelemetry-js/pull/3086) @pichlermarc * fix(exporter-grpc): prevent trace and metrics grpc service clients from interfering with each other [#3100](https://github.com/open-telemetry/opentelemetry-js/pull/3100) @dyladan * refactor trace grpc exporter to not use the grpc exporter base class +* fix(sdk-metrics-base): fix PeriodicExportingMetricReader keeping Node.js process from exiting + [#3106](https://github.com/open-telemetry/opentelemetry-js/pull/3106) @seemk ### :books: (Refine Doc) diff --git a/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts b/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts index 51886d2b233..dcf779bc694 100644 --- a/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts +++ b/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Context } from '@opentelemetry/api'; +import { Context, SpanAttributes, SpanAttributeValue } from '@opentelemetry/api'; import { BatchObservableResult, ObservableResult } from './ObservableResult'; /** @@ -82,10 +82,21 @@ export interface Histogram { record(value: number, attributes?: MetricAttributes, context?: Context): void; } +// api.SpanAttributes instead of api.Attributes is used here for api package backward compatibility. /** - * key-value pairs passed by the user. + * Attributes is a map from string to attribute values. + * + * Note: only the own enumerable keys are counted as valid attribute keys. + */ +export type MetricAttributes = SpanAttributes; + +// api.SpanAttributeValue instead of api.AttributeValue is used here for api package backward compatibility. +/** + * Attribute values may be any non-nullish primitive value except an object. + * + * null or undefined attribute values are invalid and will result in undefined behavior. */ -export type MetricAttributes = { [key: string]: string }; +export type MetricAttributeValue = SpanAttributeValue; /** * The observable callback for Observable instruments. diff --git a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts index c11ea5213c8..93955ecb4bc 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts @@ -24,7 +24,7 @@ import { DataPoint, Histogram, } from '@opentelemetry/sdk-metrics-base'; -import type { MetricAttributes } from '@opentelemetry/api-metrics'; +import type { MetricAttributes, MetricAttributeValue } from '@opentelemetry/api-metrics'; import { hrTimeToMilliseconds } from '@opentelemetry/core'; type PrometheusDataTypeLiteral = @@ -38,9 +38,15 @@ function escapeString(str: string) { return str.replace(/\\/g, '\\\\').replace(/\n/g, '\\n'); } -function escapeAttributeValue(str: string) { +/** + * String Attribute values are converted directly to Prometheus attribute values. + * Non-string values are represented as JSON-encoded strings. + * + * `undefined` is converted to an empty string. + */ +function escapeAttributeValue(str: MetricAttributeValue = '') { if (typeof str !== 'string') { - str = String(str); + str = JSON.stringify(str); } return escapeString(str).replace(/"/g, '\\"'); } diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts index daea0b209f7..45f2e69f499 100644 --- a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts +++ b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts @@ -454,12 +454,16 @@ describe('PrometheusSerializer', () => { assert.strictEqual(result, `test_total 1 ${mockedHrTimeMs}\n`); }); - it('should serialize non-string attribute values', async () => { + it('should serialize non-string attribute values in JSON representations', async () => { const serializer = new PrometheusSerializer(); const result = await testSerializer(serializer, 'test_total', counter => { counter.add(1, ({ + true: true, + false: false, + array: [1, undefined, null, 2], object: {}, + Infinity: Infinity, NaN: NaN, null: null, undefined: undefined, @@ -468,7 +472,7 @@ describe('PrometheusSerializer', () => { assert.strictEqual( result, - `test_total{object="[object Object]",NaN="NaN",null="null",undefined="undefined"} 1 ${mockedHrTimeMs}\n` + `test_total{true="true",false="false",array="[1,null,null,2]",object="{}",Infinity="null",NaN="null",null="null",undefined=""} 1 ${mockedHrTimeMs}\n` ); }); diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts index ababba83733..3e188c02931 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts @@ -15,7 +15,11 @@ */ import * as api from '@opentelemetry/api'; -import { ExportResultCode, globalErrorHandler } from '@opentelemetry/core'; +import { + ExportResultCode, + globalErrorHandler, + unrefTimer +} from '@opentelemetry/core'; import { MetricReader } from './MetricReader'; import { AggregationTemporality } from './AggregationTemporality'; import { InstrumentType } from '../InstrumentDescriptor'; @@ -100,6 +104,7 @@ export class PeriodicExportingMetricReader extends MetricReader { globalErrorHandler(err); } }, this._exportInterval); + unrefTimer(this._interval); } protected async onForceFlush(): Promise { diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts index c294eac87dc..33e198b3ba1 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts @@ -31,13 +31,9 @@ export function hashAttributes(attributes: MetricAttributes): string { let keys = Object.keys(attributes); if (keys.length === 0) return ''; + // Return a string that is stable on key orders. keys = keys.sort(); - return keys.reduce((result, key) => { - if (result.length > 2) { - result += ','; - } - return (result += key + ':' + attributes[key]); - }, '|#'); + return JSON.stringify(keys.map(key => [key, attributes[key]])); } /** diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts index cb9c9b884e0..07731a5376e 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/utils.test.ts @@ -15,8 +15,10 @@ */ import * as sinon from 'sinon'; -import { callWithTimeout, TimeoutError } from '../src/utils'; +import * as assert from 'assert'; +import { callWithTimeout, hashAttributes, TimeoutError } from '../src/utils'; import { assertRejects } from './test-utils'; +import { MetricAttributes } from '@opentelemetry/api-metrics'; describe('utils', () => { afterEach(() => { @@ -32,4 +34,23 @@ describe('utils', () => { assertRejects(callWithTimeout(promise, 100), TimeoutError); }); }); + + describe('hashAttributes', () => { + it('should hash all types of attribute values', () => { + const cases: [MetricAttributes, string][] = [ + [{ 'string': 'bar' }, '[["string","bar"]]'], + [{ 'number': 1 }, '[["number",1]]'], + [{ 'false': false, 'true': true }, '[["false",false],["true",true]]'], + [{ 'arrayOfString': ['foo','bar'] }, '[["arrayOfString",["foo","bar"]]]'], + [{ 'arrayOfNumber': [1,2] }, '[["arrayOfNumber",[1,2]]]'], + [{ 'arrayOfBool': [false,true] }, '[["arrayOfBool",[false,true]]]'], + [{ 'undefined': undefined }, '[["undefined",null]]'], + [{ 'arrayOfHoles': [undefined, null] }, '[["arrayOfHoles",[null,null]]]'], + ]; + + for (const [idx, it] of cases.entries()) { + assert.strictEqual(hashAttributes(it[0]), it[1], `cases[${idx}] failed`); + } + }); + }); }); diff --git a/experimental/packages/otlp-transformer/test/metrics.test.ts b/experimental/packages/otlp-transformer/test/metrics.test.ts index beb9785f704..bc1d85bf22c 100644 --- a/experimental/packages/otlp-transformer/test/metrics.test.ts +++ b/experimental/packages/otlp-transformer/test/metrics.test.ts @@ -29,6 +29,13 @@ import { hrTime, hrTimeToNanoseconds } from '@opentelemetry/core'; const START_TIME = hrTime(); const END_TIME = hrTime(); +const ATTRIBUTES = { + 'string-attribute': 'some attribute value', + 'int-attribute': 1, + 'double-attribute': 1.1, + 'boolean-attribute': true, + 'array-attribute': ['attribute value 1', 'attribute value 2'], +}; describe('Metrics', () => { describe('createExportMetricsServiceRequest', () => { @@ -58,6 +65,39 @@ describe('Metrics', () => { stringValue: 'some attribute value', }, }, + { + key: 'int-attribute', + value: { + intValue: 1, + }, + }, + { + key: 'double-attribute', + value: { + doubleValue: 1.1, + }, + }, + { + key: 'boolean-attribute', + value: { + boolValue: true, + }, + }, + { + key: 'array-attribute', + value: { + arrayValue: { + values: [ + { + stringValue: 'attribute value 1', + }, + { + stringValue: 'attribute value 2', + } + ] + }, + }, + }, ]; function createCounterData(value: number, aggregationTemporality: AggregationTemporality): MetricData { @@ -77,7 +117,7 @@ describe('Metrics', () => { value: value, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' } + attributes: ATTRIBUTES, } ] }; @@ -100,7 +140,7 @@ describe('Metrics', () => { value: value, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' } + attributes: ATTRIBUTES } ] }; @@ -123,7 +163,7 @@ describe('Metrics', () => { value: value, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' } + attributes: ATTRIBUTES, } ] }; @@ -146,7 +186,7 @@ describe('Metrics', () => { value: value, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' } + attributes: ATTRIBUTES, } ] }; @@ -169,7 +209,7 @@ describe('Metrics', () => { value: value, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' } + attributes: ATTRIBUTES, } ] }; @@ -207,7 +247,7 @@ describe('Metrics', () => { }, startTime: START_TIME, endTime: END_TIME, - attributes: { 'string-attribute': 'some attribute value' }, + attributes: ATTRIBUTES, } ] }; diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index a964de5840f..b0560fc3f15 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -202,12 +202,23 @@ export class BasicTracerProvider implements TracerProvider { return this.activeSpanProcessor.shutdown(); } + /** + * TS cannot yet infer the type of this.constructor: + * https://github.com/Microsoft/TypeScript/issues/3841#issuecomment-337560146 + * There is no need to override either of the getters in your child class. + * The type of the registered component maps should be the same across all + * classes in the inheritance tree. + */ protected _getPropagator(name: string): TextMapPropagator | undefined { - return BasicTracerProvider._registeredPropagators.get(name)?.(); + return ( + (this.constructor as typeof BasicTracerProvider)._registeredPropagators + ).get(name)?.(); } protected _getSpanExporter(name: string): SpanExporter | undefined { - return BasicTracerProvider._registeredExporters.get(name)?.(); + return ( + (this.constructor as typeof BasicTracerProvider)._registeredExporters + ).get(name)?.(); } protected _buildPropagatorFromEnv(): TextMapPropagator | undefined { diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index e2b17bb5614..8575b044f67 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -29,9 +29,10 @@ import { } from '@opentelemetry/api'; import { CompositePropagator } from '@opentelemetry/core'; import { - AlwaysOnSampler, AlwaysOffSampler, + AlwaysOnSampler, TraceState, + W3CTraceContextPropagator, } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; import * as assert from 'assert'; @@ -45,10 +46,35 @@ import { BatchSpanProcessor, } from '../../src'; -describe('BasicTracerProvider', () => { - let removeEvent: (() => void) | undefined; +class DummyPropagator implements TextMapPropagator { + inject( + context: Context, + carrier: any, + setter: TextMapSetter + ): void { + throw new Error('Method not implemented.'); + } + extract( + context: Context, + carrier: any, + getter: TextMapGetter + ): Context { + throw new Error('Method not implemented.'); + } + fields(): string[] { + throw new Error('Method not implemented.'); + } +} + +class DummyExporter extends InMemorySpanExporter {} +describe('BasicTracerProvider', () => { let envSource: Record; + let setGlobalPropagatorStub: sinon.SinonSpy< + [TextMapPropagator], + boolean + >; + if (typeof process === 'undefined') { envSource = (globalThis as unknown) as Record; } else { @@ -56,15 +82,15 @@ describe('BasicTracerProvider', () => { } beforeEach(() => { + // to avoid actually registering the TraceProvider and leaking env to other tests + sinon.stub(trace, 'setGlobalTracerProvider'); + setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); + context.disable(); }); afterEach(() => { sinon.restore(); - if (removeEvent) { - removeEvent(); - removeEvent = undefined; - } }); describe('constructor', () => { @@ -240,40 +266,104 @@ describe('BasicTracerProvider', () => { }); }); - describe('.register()', () => { - describe('propagator', () => { - class DummyPropagator implements TextMapPropagator { - inject( - context: Context, - carrier: any, - setter: TextMapSetter - ): void { - throw new Error('Method not implemented.'); - } - extract( - context: Context, - carrier: any, - getter: TextMapGetter - ): Context { - throw new Error('Method not implemented.'); + describe('Custom TracerProvider through inheritance', () => { + beforeEach(() => { + envSource.OTEL_TRACES_EXPORTER = 'custom-exporter'; + envSource.OTEL_PROPAGATORS = 'custom-propagator'; + }); + + afterEach(() => { + delete envSource.OTEL_TRACES_EXPORTER; + delete envSource.OTEL_PROPAGATORS; + sinon.restore(); + }); + + it('can be extended by overriding registered components', () => { + class CustomTracerProvider extends BasicTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ...BasicTracerProvider._registeredPropagators, + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ...BasicTracerProvider._registeredExporters, + ['custom-exporter', () => new DummyExporter()], + ]); + } + + const provider = new CustomTracerProvider({}); + assert(provider['_getPropagator']('tracecontext') instanceof W3CTraceContextPropagator); + /* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */ + + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + + it('the old way of extending still works', () => { + // this is an anti-pattern, but we test that for backwards compatibility + class CustomTracerProvider extends BasicTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => new DummyPropagator()], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + + protected override _getPropagator(name: string): TextMapPropagator | undefined { + return ( + super._getPropagator(name) || + CustomTracerProvider._registeredPropagators.get(name)?.() + ); } - fields(): string[] { - throw new Error('Method not implemented.'); + + protected override _getSpanExporter(name: string): SpanExporter | undefined { + return ( + super._getSpanExporter(name) || + CustomTracerProvider._registeredExporters.get(name)?.() + ); } } - let setGlobalPropagatorStub: sinon.SinonSpy< - [TextMapPropagator], - boolean - >; + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + sinon.assert.calledOnceWithExactly(setGlobalPropagatorStub, sinon.match.instanceOf(DummyPropagator)); + }); + }); + + describe('.register()', () => { + describe('propagator', () => { let originalPropagators: string | number | undefined | string[]; beforeEach(() => { - setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator'); originalPropagators = envSource.OTEL_PROPAGATORS; }); afterEach(() => { - setGlobalPropagatorStub.restore(); + sinon.restore(); // otherwise we may assign 'undefined' (a string) if (originalPropagators !== undefined) { @@ -288,10 +378,10 @@ describe('BasicTracerProvider', () => { provider.register({ propagator: new DummyPropagator(), }); - assert.ok( - setGlobalPropagatorStub.calledOnceWithExactly( - sinon.match.instanceOf(DummyPropagator) - ) + + sinon.assert.calledOnceWithExactly( + setGlobalPropagatorStub, + sinon.match.instanceOf(DummyPropagator) ); }); @@ -299,10 +389,9 @@ describe('BasicTracerProvider', () => { const provider = new BasicTracerProvider(); provider.register(); - assert.ok( - setGlobalPropagatorStub.calledOnceWithExactly( - sinon.match.instanceOf(CompositePropagator) - ) + sinon.assert.calledOnceWithExactly( + setGlobalPropagatorStub, + sinon.match.instanceOf(CompositePropagator) ); assert.deepStrictEqual(setGlobalPropagatorStub.args[0][0].fields(), [ 'traceparent', @@ -318,13 +407,10 @@ describe('BasicTracerProvider', () => { const provider = new BasicTracerProvider({}); provider.register(); - assert.ok( - warnStub.calledOnceWithExactly( - 'Propagator "missing-propagator" requested through environment variable is unavailable.' - ) + sinon.assert.calledOnceWithExactly( + warnStub, + 'Propagator "missing-propagator" requested through environment variable is unavailable.' ); - - warnStub.restore(); }); }); diff --git a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts index 8e4575a390c..9d552162f64 100644 --- a/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { TextMapPropagator } from '@opentelemetry/api'; import { AsyncHooksContextManager, AsyncLocalStorageContextManager, @@ -40,6 +39,7 @@ export class NodeTracerProvider extends BasicTracerProvider { string, PROPAGATOR_FACTORY >([ + ...BasicTracerProvider._registeredPropagators, [ 'b3', () => @@ -67,11 +67,4 @@ export class NodeTracerProvider extends BasicTracerProvider { super.register(config); } - - protected override _getPropagator(name: string): TextMapPropagator | undefined { - return ( - super._getPropagator(name) || - NodeTracerProvider._registeredPropagators.get(name)?.() - ); - } } diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index 0d53e25eb6e..24ba20f4b03 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -14,19 +14,32 @@ * limitations under the License. */ +import * as sinon from 'sinon'; +import * as assert from 'assert'; + import { context, + Context, + ContextManager, + propagation, + ROOT_CONTEXT, + TextMapGetter, + TextMapPropagator, + TextMapSetter, + trace, TraceFlags, - propagation, trace, } from '@opentelemetry/api'; import { AlwaysOnSampler, AlwaysOffSampler } from '@opentelemetry/core'; import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; -import { Span } from '@opentelemetry/sdk-trace-base'; +import { + BatchSpanProcessor, + InMemorySpanExporter, + Span, + SpanExporter, +} from '@opentelemetry/sdk-trace-base'; import { Resource } from '@opentelemetry/resources'; import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'; -import * as assert from 'assert'; -import * as path from 'path'; -import { ContextManager, ROOT_CONTEXT } from '@opentelemetry/api'; + import { NodeTracerProvider } from '../src/NodeTracerProvider'; const sleep = (time: number) => @@ -34,18 +47,9 @@ const sleep = (time: number) => return setTimeout(resolve, time); }); -const INSTALLED_PLUGINS_PATH = path.join( - __dirname, - 'instrumentation', - 'node_modules' -); - describe('NodeTracerProvider', () => { let provider: NodeTracerProvider; let contextManager: ContextManager; - before(() => { - module.paths.push(INSTALLED_PLUGINS_PATH); - }); beforeEach(() => { contextManager = new AsyncHooksContextManager(); @@ -53,8 +57,6 @@ describe('NodeTracerProvider', () => { }); afterEach(() => { - // clear require cache - Object.keys(require.cache).forEach(key => delete require.cache[key]); contextManager.disable(); context.disable(); }); @@ -243,4 +245,122 @@ describe('NodeTracerProvider', () => { ]); }); }); + + + describe('Custom TracerProvider through inheritance', () => { + class DummyPropagator implements TextMapPropagator { + inject( + context: Context, + carrier: any, + setter: TextMapSetter + ): void { + throw new Error('Method not implemented.'); + } + extract( + context: Context, + carrier: any, + getter: TextMapGetter + ): Context { + throw new Error('Method not implemented.'); + } + fields(): string[] { + throw new Error('Method not implemented.'); + } + } + + class DummyExporter extends InMemorySpanExporter {} + + beforeEach(() => { + process.env.OTEL_TRACES_EXPORTER = 'custom-exporter'; + process.env.OTEL_PROPAGATORS = 'custom-propagator'; + + propagation.disable(); + trace.disable(); + }); + + afterEach(() => { + delete process.env.OTEL_TRACES_EXPORTER; + delete process.env.OTEL_PROPAGATORS; + + propagation.disable(); + trace.disable(); + + sinon.restore(); + }); + + it('can be extended by overriding registered components', () => { + const propagator = new DummyPropagator(); + + class CustomTracerProvider extends NodeTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => propagator], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + } + + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); + }); + + it('the old way of extending still works', () => { + const propagator = new DummyPropagator(); + + // this is an anti-pattern, but we test that for backwards compatibility + class CustomTracerProvider extends NodeTracerProvider { + protected static override readonly _registeredPropagators = new Map< + string, + () => TextMapPropagator + >([ + ['custom-propagator', () => propagator], + ]); + + protected static override readonly _registeredExporters = new Map< + string, + () => SpanExporter + >([ + ['custom-exporter', () => new DummyExporter()], + ]); + + protected override _getPropagator(name: string): TextMapPropagator | undefined { + return ( + super._getPropagator(name) || + CustomTracerProvider._registeredPropagators.get(name)?.() + ); + } + + protected override _getSpanExporter(name: string): SpanExporter | undefined { + return ( + super._getSpanExporter(name) || + CustomTracerProvider._registeredExporters.get(name)?.() + ); + } + } + + const provider = new CustomTracerProvider({}); + provider.register(); + const processor = provider.getActiveSpanProcessor(); + assert(processor instanceof BatchSpanProcessor); + // @ts-expect-error access configured to verify its the correct one + const exporter = processor._exporter; + assert(exporter instanceof DummyExporter); + + assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); + }); + }); }); diff --git a/packages/opentelemetry-sdk-trace-node/test/registration.test.ts b/packages/opentelemetry-sdk-trace-node/test/registration.test.ts index 4db2d334b4b..09cc24b327a 100644 --- a/packages/opentelemetry-sdk-trace-node/test/registration.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/registration.test.ts @@ -14,6 +14,9 @@ * limitations under the License. */ +import * as assert from 'assert'; +import { inspect } from 'util'; + import { context, propagation, @@ -25,10 +28,16 @@ import { AsyncLocalStorageContextManager, } from '@opentelemetry/context-async-hooks'; import { CompositePropagator } from '@opentelemetry/core'; -import * as assert from 'assert'; import { NodeTracerProvider } from '../src'; import * as semver from 'semver'; +const assertInstanceOf = (actual: object, ExpectedInstance: Function) => { + assert.ok( + actual instanceof ExpectedInstance, + `Expected ${inspect(actual)} to be instance of ${ExpectedInstance.name}` + ); +}; + const DefaultContextManager = semver.gte(process.version, '14.8.0') ? AsyncLocalStorageContextManager : AsyncHooksContextManager; @@ -44,9 +53,9 @@ describe('API registration', () => { const tracerProvider = new NodeTracerProvider(); tracerProvider.register(); - assert.ok(context['_getContextManager']() instanceof DefaultContextManager); - assert.ok( - propagation['_getGlobalPropagator']() instanceof CompositePropagator + assertInstanceOf(context['_getContextManager'](), DefaultContextManager); + assertInstanceOf( + propagation['_getGlobalPropagator'](), CompositePropagator ); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; @@ -80,8 +89,8 @@ describe('API registration', () => { assert.strictEqual(context['_getContextManager'](), ctxManager, 'context manager should not change'); - assert.ok( - propagation['_getGlobalPropagator']() instanceof CompositePropagator + assertInstanceOf( + propagation['_getGlobalPropagator'](), CompositePropagator ); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; @@ -98,7 +107,7 @@ describe('API registration', () => { assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); - assert.ok(context['_getContextManager']() instanceof DefaultContextManager); + assertInstanceOf(context['_getContextManager'](), DefaultContextManager); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; assert.ok(apiTracerProvider.getDelegate() === tracerProvider);