From d808e2957f84d127db1a5a3b283227cd21a28719 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 26 Oct 2022 15:36:52 -0400 Subject: [PATCH] Add semver check to metrics API (#3357) --- experimental/CHANGELOG.md | 3 + .../src/api/global-utils.ts | 55 ------- .../src/api/metrics.ts | 32 +--- .../src/internal/global-utils.ts | 96 ++++++++++++ .../src/internal/semver.ts | 140 +++++++++++++++++ .../test/{api => internal}/global.test.ts | 66 ++++++-- .../test/internal/semver.test.ts | 148 ++++++++++++++++++ .../test/internal/version.test.ts | 30 ++++ 8 files changed, 478 insertions(+), 92 deletions(-) delete mode 100644 experimental/packages/opentelemetry-api-metrics/src/api/global-utils.ts create mode 100644 experimental/packages/opentelemetry-api-metrics/src/internal/global-utils.ts create mode 100644 experimental/packages/opentelemetry-api-metrics/src/internal/semver.ts rename experimental/packages/opentelemetry-api-metrics/test/{api => internal}/global.test.ts (50%) create mode 100644 experimental/packages/opentelemetry-api-metrics/test/internal/semver.test.ts create mode 100644 experimental/packages/opentelemetry-api-metrics/test/internal/version.test.ts diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 7913109e53..6d2de9fb60 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to experimental packages in this project will be documented ### :boom: Breaking Change +* Add semver check to metrics API [#3357](https://github.com/open-telemetry/opentelemetry-js/pull/3357) @dyladan + * Previously API versions were only considered compatible if the API was exactly the same + ### :rocket: (Enhancement) * feat(metrics-sdk): Add tracing suppresing for Metrics Export [#3332](https://github.com/open-telemetry/opentelemetry-js/pull/3332) @hectorhdzg diff --git a/experimental/packages/opentelemetry-api-metrics/src/api/global-utils.ts b/experimental/packages/opentelemetry-api-metrics/src/api/global-utils.ts deleted file mode 100644 index e371d5165d..0000000000 --- a/experimental/packages/opentelemetry-api-metrics/src/api/global-utils.ts +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { MeterProvider } from '../types/MeterProvider'; -import { _globalThis } from '../platform'; - -export const GLOBAL_METRICS_API_KEY = Symbol.for( - 'io.opentelemetry.js.api.metrics' -); - -type Get = (version: number) => T; -type OtelGlobal = Partial<{ - [GLOBAL_METRICS_API_KEY]: Get; -}>; - -export const _global = _globalThis as OtelGlobal; - -/** - * Make a function which accepts a version integer and returns the instance of an API if the version - * is compatible, or a fallback version (usually NOOP) if it is not. - * - * @param requiredVersion Backwards compatibility version which is required to return the instance - * @param instance Instance which should be returned if the required version is compatible - * @param fallback Fallback instance, usually NOOP, which will be returned if the required version is not compatible - */ -export function makeGetter( - requiredVersion: number, - instance: T, - fallback: T -): Get { - return (version: number): T => - version === requiredVersion ? instance : fallback; -} - -/** - * A number which should be incremented each time a backwards incompatible - * change is made to the API. This number is used when an API package - * attempts to access the global API to ensure it is getting a compatible - * version. If the global API is not compatible with the API package - * attempting to get it, a NOOP API implementation will be returned. - */ -export const API_BACKWARDS_COMPATIBILITY_VERSION = 4; diff --git a/experimental/packages/opentelemetry-api-metrics/src/api/metrics.ts b/experimental/packages/opentelemetry-api-metrics/src/api/metrics.ts index 3e5fb6015a..b3f9bac6b6 100644 --- a/experimental/packages/opentelemetry-api-metrics/src/api/metrics.ts +++ b/experimental/packages/opentelemetry-api-metrics/src/api/metrics.ts @@ -17,12 +17,7 @@ import { Meter, MeterOptions } from '../types/Meter'; import { MeterProvider } from '../types/MeterProvider'; import { NOOP_METER_PROVIDER } from '../NoopMeterProvider'; -import { - API_BACKWARDS_COMPATIBILITY_VERSION, - GLOBAL_METRICS_API_KEY, - makeGetter, - _global, -} from './global-utils'; +import { getGlobal, registerGlobal, unregisterGlobal } from '../internal/global-utils'; /** * Singleton object which represents the entry point to the OpenTelemetry Metrics API @@ -43,31 +38,18 @@ export class MetricsAPI { } /** - * Set the current global meter. Returns the initialized global meter provider. + * Set the current global meter provider. + * Returns true if the meter provider was successfully registered, else false. */ - public setGlobalMeterProvider(provider: MeterProvider): MeterProvider { - if (_global[GLOBAL_METRICS_API_KEY]) { - // global meter provider has already been set - return this.getMeterProvider(); - } - - _global[GLOBAL_METRICS_API_KEY] = makeGetter( - API_BACKWARDS_COMPATIBILITY_VERSION, - provider, - NOOP_METER_PROVIDER - ); - - return provider; + public setGlobalMeterProvider(provider: MeterProvider): boolean { + return registerGlobal('metrics', provider); } /** * Returns the global meter provider. */ public getMeterProvider(): MeterProvider { - return ( - _global[GLOBAL_METRICS_API_KEY]?.(API_BACKWARDS_COMPATIBILITY_VERSION) ?? - NOOP_METER_PROVIDER - ); + return getGlobal('metrics') || NOOP_METER_PROVIDER; } /** @@ -79,6 +61,6 @@ export class MetricsAPI { /** Remove the global meter provider */ public disable(): void { - delete _global[GLOBAL_METRICS_API_KEY]; + unregisterGlobal('metrics'); } } diff --git a/experimental/packages/opentelemetry-api-metrics/src/internal/global-utils.ts b/experimental/packages/opentelemetry-api-metrics/src/internal/global-utils.ts new file mode 100644 index 0000000000..f83c203398 --- /dev/null +++ b/experimental/packages/opentelemetry-api-metrics/src/internal/global-utils.ts @@ -0,0 +1,96 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { diag } from '@opentelemetry/api'; +import { _globalThis } from '../platform'; +import { MeterProvider } from '../types/MeterProvider'; +import { VERSION } from '../version'; +import { isCompatible } from './semver'; + +const major = VERSION.split('.')[0]; +const GLOBAL_OPENTELEMETRY_METRICS_API_KEY = Symbol.for( + `opentelemetry.js.api.metrics.${major}` +); + +const _global = _globalThis as OTelGlobal; + +export function registerGlobal( + type: Type, + instance: OTelGlobalAPI[Type], + allowOverride = false +): boolean { + const api = (_global[GLOBAL_OPENTELEMETRY_METRICS_API_KEY] = _global[ + GLOBAL_OPENTELEMETRY_METRICS_API_KEY + ] ?? { + version: VERSION, + }); + + if (!allowOverride && api[type]) { + // already registered an API of this type + const err = new Error( + `@opentelemetry/api: Attempted duplicate registration of API: ${type}` + ); + diag.error(err.stack || err.message); + return false; + } + + if (api.version !== VERSION) { + // All registered APIs must be of the same version exactly + const err = new Error( + '@opentelemetry/api: All API registration versions must match' + ); + diag.error(err.stack || err.message); + return false; + } + + api[type] = instance; + diag.debug( + `@opentelemetry/api: Registered a global for ${type} v${VERSION}.` + ); + + return true; +} + +export function getGlobal( + type: Type +): OTelGlobalAPI[Type] | undefined { + const globalVersion = _global[GLOBAL_OPENTELEMETRY_METRICS_API_KEY]?.version; + if (!globalVersion || !isCompatible(globalVersion)) { + return; + } + return _global[GLOBAL_OPENTELEMETRY_METRICS_API_KEY]?.[type]; +} + +export function unregisterGlobal(type: keyof OTelGlobalAPI) { + diag.debug( + `@opentelemetry/api-metrics: Unregistering a global for ${type} v${VERSION}.` + ); + const api = _global[GLOBAL_OPENTELEMETRY_METRICS_API_KEY]; + + if (api) { + delete api[type]; + } +} + +type OTelGlobal = { + [GLOBAL_OPENTELEMETRY_METRICS_API_KEY]?: OTelGlobalAPI; +}; + +type OTelGlobalAPI = { + version: string; + + metrics?: MeterProvider; +}; diff --git a/experimental/packages/opentelemetry-api-metrics/src/internal/semver.ts b/experimental/packages/opentelemetry-api-metrics/src/internal/semver.ts new file mode 100644 index 0000000000..076f9b9b51 --- /dev/null +++ b/experimental/packages/opentelemetry-api-metrics/src/internal/semver.ts @@ -0,0 +1,140 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { VERSION } from '../version'; + +const re = /^(\d+)\.(\d+)\.(\d+)(-(.+))?$/; + +/** + * Create a function to test an API version to see if it is compatible with the provided ownVersion. + * + * The returned function has the following semantics: + * - Exact match is always compatible + * - Major versions must match exactly + * - 1.x package cannot use global 2.x package + * - 2.x package cannot use global 1.x package + * - The minor version of the API module requesting access to the global API must be less than or equal to the minor version of this API + * - 1.3 package may use 1.4 global because the later global contains all functions 1.3 expects + * - 1.4 package may NOT use 1.3 global because it may try to call functions which don't exist on 1.3 + * - If the major version is 0, the minor version is treated as the major and the patch is treated as the minor + * - Patch and build tag differences are not considered at this time + * + * @param ownVersion version which should be checked against + */ +export function _makeCompatibilityCheck( + ownVersion: string +): (globalVersion: string) => boolean { + const acceptedVersions = new Set([ownVersion]); + const rejectedVersions = new Set(); + + const myVersionMatch = ownVersion.match(re); + if (!myVersionMatch) { + // we cannot guarantee compatibility so we always return noop + return () => false; + } + + const ownVersionParsed = { + major: +myVersionMatch[1], + minor: +myVersionMatch[2], + patch: +myVersionMatch[3], + prerelease: myVersionMatch[4], + }; + + // if ownVersion has a prerelease tag, versions must match exactly + if (ownVersionParsed.prerelease != null) { + return function isExactmatch(globalVersion: string): boolean { + return globalVersion === ownVersion; + }; + } + + function _reject(v: string) { + rejectedVersions.add(v); + return false; + } + + function _accept(v: string) { + acceptedVersions.add(v); + return true; + } + + return function isCompatible(globalVersion: string): boolean { + if (acceptedVersions.has(globalVersion)) { + return true; + } + + if (rejectedVersions.has(globalVersion)) { + return false; + } + + const globalVersionMatch = globalVersion.match(re); + if (!globalVersionMatch) { + // cannot parse other version + // we cannot guarantee compatibility so we always noop + return _reject(globalVersion); + } + + const globalVersionParsed = { + major: +globalVersionMatch[1], + minor: +globalVersionMatch[2], + patch: +globalVersionMatch[3], + prerelease: globalVersionMatch[4], + }; + + // if globalVersion has a prerelease tag, versions must match exactly + if (globalVersionParsed.prerelease != null) { + return _reject(globalVersion); + } + + // major versions must match + if (ownVersionParsed.major !== globalVersionParsed.major) { + return _reject(globalVersion); + } + + if (ownVersionParsed.major === 0) { + if ( + ownVersionParsed.minor === globalVersionParsed.minor && + ownVersionParsed.patch <= globalVersionParsed.patch + ) { + return _accept(globalVersion); + } + + return _reject(globalVersion); + } + + if (ownVersionParsed.minor <= globalVersionParsed.minor) { + return _accept(globalVersion); + } + + return _reject(globalVersion); + }; +} + +/** + * Test an API version to see if it is compatible with this API. + * + * - Exact match is always compatible + * - Major versions must match exactly + * - 1.x package cannot use global 2.x package + * - 2.x package cannot use global 1.x package + * - The minor version of the API module requesting access to the global API must be less than or equal to the minor version of this API + * - 1.3 package may use 1.4 global because the later global contains all functions 1.3 expects + * - 1.4 package may NOT use 1.3 global because it may try to call functions which don't exist on 1.3 + * - If the major version is 0, the minor version is treated as the major and the patch is treated as the minor + * - Patch and build tag differences are not considered at this time + * + * @param version version of the API requesting an instance of the global API + */ +export const isCompatible = _makeCompatibilityCheck(VERSION); diff --git a/experimental/packages/opentelemetry-api-metrics/test/api/global.test.ts b/experimental/packages/opentelemetry-api-metrics/test/internal/global.test.ts similarity index 50% rename from experimental/packages/opentelemetry-api-metrics/test/api/global.test.ts rename to experimental/packages/opentelemetry-api-metrics/test/internal/global.test.ts index c48949a3e5..69fbee57c6 100644 --- a/experimental/packages/opentelemetry-api-metrics/test/api/global.test.ts +++ b/experimental/packages/opentelemetry-api-metrics/test/internal/global.test.ts @@ -15,8 +15,11 @@ */ import * as assert from 'assert'; -import { _global, GLOBAL_METRICS_API_KEY } from '../../src/api/global-utils'; +import { getGlobal } from '../../src/internal/global-utils'; +import { _globalThis } from '../../src/platform'; import { NoopMeterProvider } from '../../src/NoopMeterProvider'; +import sinon = require('sinon'); +import { diag } from '@opentelemetry/api'; const api1 = require('../../src') as typeof import('../../src'); @@ -26,18 +29,31 @@ for (const key of Object.keys(require.cache)) { } const api2 = require('../../src') as typeof import('../../src'); +// This will need to be changed manually on major version changes. +// It is intentionally not autogenerated to ensure the author of the change is aware of what they are doing. +const GLOBAL_METRICS_API_SYMBOL_KEY = 'opentelemetry.js.api.metrics.0'; + +const getMockLogger = () => ({ + verbose: sinon.spy(), + debug: sinon.spy(), + info: sinon.spy(), + warn: sinon.spy(), + error: sinon.spy(), +}); + describe('Global Utils', () => { // prove they are separate instances - assert.notStrictEqual(api1, api2); + assert.notEqual(api1, api2); // that return separate noop instances to start assert.notStrictEqual( api1.metrics.getMeterProvider(), - api2.metrics.getMeterProvider() + api2.metrics.getMeterProvider(), ); beforeEach(() => { api1.metrics.disable(); - api2.metrics.disable(); + // @ts-expect-error we are modifying internals for testing purposes here + delete _globalThis[Symbol.for(GLOBAL_METRICS_API_SYMBOL_KEY)]; }); it('should change the global meter provider', () => { @@ -57,20 +73,46 @@ describe('Global Utils', () => { }); it('should disable both if one is disabled', () => { - const original = api1.metrics.getMeterProvider(); + const manager = new NoopMeterProvider(); + api1.metrics.setGlobalMeterProvider(manager); - api1.metrics.setGlobalMeterProvider(new NoopMeterProvider()); - - assert.notStrictEqual(original, api1.metrics.getMeterProvider()); + assert.strictEqual(manager, api1.metrics.getMeterProvider()); api2.metrics.disable(); - assert.strictEqual(original, api1.metrics.getMeterProvider()); + assert.notStrictEqual(manager, api1.metrics.getMeterProvider()); }); it('should return the module NoOp implementation if the version is a mismatch', () => { - const original = api1.metrics.getMeterProvider(); + const newMeterProvider = new NoopMeterProvider(); + api1.metrics.setGlobalMeterProvider(newMeterProvider); + + // ensure new meter provider is returned + assert.strictEqual(api1.metrics.getMeterProvider(), newMeterProvider); + + const globalInstance = getGlobal('metrics'); + assert.ok(globalInstance); + // @ts-expect-error we are modifying internals for testing purposes here + _globalThis[Symbol.for(GLOBAL_METRICS_API_SYMBOL_KEY)].version = '0.0.1'; + + // ensure new meter provider is not returned because version above is incompatible + assert.notStrictEqual( + api1.metrics.getMeterProvider(), + newMeterProvider + ); + }); + + it('should log an error if there is a duplicate registration', () => { + const logger = getMockLogger(); + diag.setLogger(logger); + + api1.metrics.setGlobalMeterProvider(new NoopMeterProvider()); api1.metrics.setGlobalMeterProvider(new NoopMeterProvider()); - const afterSet = _global[GLOBAL_METRICS_API_KEY]!(-1); - assert.strictEqual(original, afterSet); + sinon.assert.calledOnce(logger.error); + assert.strictEqual(logger.error.firstCall.args.length, 1); + assert.ok( + logger.error.firstCall.args[0].startsWith( + 'Error: @opentelemetry/api: Attempted duplicate registration of API: metrics' + ) + ); }); }); diff --git a/experimental/packages/opentelemetry-api-metrics/test/internal/semver.test.ts b/experimental/packages/opentelemetry-api-metrics/test/internal/semver.test.ts new file mode 100644 index 0000000000..e9ac3ec727 --- /dev/null +++ b/experimental/packages/opentelemetry-api-metrics/test/internal/semver.test.ts @@ -0,0 +1,148 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert'; +import { + isCompatible, + _makeCompatibilityCheck, +} from '../../src/internal/semver'; +import { VERSION } from '../../src/version'; + +describe('semver', () => { + it('should be compatible if versions are equal', () => { + assert.ok(isCompatible(VERSION)); + }); + + it('returns false if own version cannot be parsed', () => { + const check = _makeCompatibilityCheck('this is not semver'); + assert.ok(!check('1.0.0')); + }); + + it('incompatible if other version cannot be parsed', () => { + const check = _makeCompatibilityCheck('0.1.2'); + assert.ok(!check('this is not semver')); + }); + + describe('>= 1.0.0', () => { + const globalVersion = '5.5.5'; + const vers: [string, boolean][] = [ + // same major/minor run should be compatible + ['5.5.5', true], + ['5.5.6', true], + ['5.5.4', true], + + // prerelease version should not be compatible + ['5.5.5-rc.0', false], + + // if our version has a minor version increase, we may try to call methods which don't exist on the global + ['5.6.5', false], + ['5.6.6', false], + ['5.6.4', false], + + // if the global version is ahead of us by a minor revision, it has at least the methods we know about + ['5.4.5', true], + ['5.4.6', true], + ['5.4.4', true], + + // major version mismatch is always incompatible + ['6.5.5', false], + ['6.5.6', false], + ['6.5.4', false], + ['6.6.5', false], + ['6.6.6', false], + ['6.6.4', false], + ['6.4.5', false], + ['6.4.6', false], + ['6.4.4', false], + ['4.5.5', false], + ['4.5.6', false], + ['4.5.4', false], + ['4.6.5', false], + ['4.6.6', false], + ['4.6.4', false], + ['4.4.5', false], + ['4.4.6', false], + ['4.4.4', false], + ]; + + test(globalVersion, vers); + }); + + describe('< 1.0.0', () => { + const globalVersion = '0.5.5'; + const vers: [string, boolean][] = [ + // same minor/patch should be compatible + ['0.5.5', true], + + // prerelease version should not be compatible + ['0.5.5-rc.0', false], + + // if our version has a patch version increase, we may try to call methods which don't exist on the global + ['0.5.6', false], + + // if the global version is ahead of us by a patch revision, it has at least the methods we know about + ['0.5.4', true], + + // minor version mismatch is always incompatible + ['0.6.5', false], + ['0.6.6', false], + ['0.6.4', false], + ['0.4.5', false], + ['0.4.6', false], + ['0.4.4', false], + + // major version mismatch is always incompatible + ['1.5.5', false], + ['1.5.6', false], + ['1.5.4', false], + ['1.6.5', false], + ['1.6.6', false], + ['1.6.4', false], + ['1.4.5', false], + ['1.4.6', false], + ['1.4.4', false], + ]; + + test(globalVersion, vers); + }); + + describe('global version is prerelease', () => { + const globalVersion = '1.0.0-rc.3'; + const vers: [string, boolean][] = [ + // must match exactly + ['1.0.0', false], + ['1.0.0-rc.2', false], + ['1.0.0-rc.4', false], + + ['1.0.0-rc.3', true], + ]; + + test(globalVersion, vers); + }); +}); + +function test(globalVersion: string, vers: [string, boolean][]) { + describe(`global version is ${globalVersion}`, () => { + for (const [version, compatible] of vers) { + it(`API version ${version} ${ + compatible ? 'should' : 'should not' + } be able to access global`, () => { + const check = _makeCompatibilityCheck(version); + assert.strictEqual(check(globalVersion), compatible); + }); + } + }); +} diff --git a/experimental/packages/opentelemetry-api-metrics/test/internal/version.test.ts b/experimental/packages/opentelemetry-api-metrics/test/internal/version.test.ts new file mode 100644 index 0000000000..4d0e5f2542 --- /dev/null +++ b/experimental/packages/opentelemetry-api-metrics/test/internal/version.test.ts @@ -0,0 +1,30 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert'; +import { VERSION } from '../../src/version'; + +describe('version', () => { + it('should have generated VERSION.ts', () => { + const pjson = require('../../package.json'); + assert.strictEqual(pjson.version, VERSION); + }); + + it('prerelease tag versions are banned', () => { + // see https://github.com/open-telemetry/opentelemetry-js-api/issues/74 + assert.ok(VERSION.match(/^\d+\.\d+\.\d+$/)); + }); +});