From 55f15e90d6cb2e455e03c0c4b72f64b023afffaa Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Tue, 18 Jun 2024 16:17:36 +0200 Subject: [PATCH] feat(sdk-metrics)!: replace attributeKeys with an option to add custom processors (#4532) --- CHANGELOG_NEXT.md | 2 + packages/sdk-metrics/src/index.ts | 6 ++ .../src/state/AsyncMetricStorage.ts | 4 +- .../sdk-metrics/src/state/MeterSharedState.ts | 9 +- .../src/state/SyncMetricStorage.ts | 4 +- .../src/view/AttributesProcessor.ts | 98 +++++++++++++++---- packages/sdk-metrics/src/view/View.ts | 32 +++--- .../sdk-metrics/test/MeterProvider.test.ts | 7 +- .../test/state/AsyncMetricStorage.test.ts | 14 +-- .../test/state/SyncMetricStorage.test.ts | 8 +- .../test/view/AttributesProcessor.test.ts | 81 +++++++++++++-- packages/sdk-metrics/test/view/View.test.ts | 16 ++- 12 files changed, 215 insertions(+), 66 deletions(-) diff --git a/CHANGELOG_NEXT.md b/CHANGELOG_NEXT.md index 7e9bf00451..d8949608e1 100644 --- a/CHANGELOG_NEXT.md +++ b/CHANGELOG_NEXT.md @@ -6,6 +6,8 @@ * chore(otel-core): replace deprecated SpanAttributes [#4408](https://github.com/open-telemetry/opentelemetry-js/pull/4408) @JamieDanielson * feat(sdk-metrics)!: remove MeterProvider.addMetricReader() in favor of constructor option [#4419](https://github.com/open-telemetry/opentelemetry-js/pull/4419) @pichlermarc * chore(otel-resources): replace deprecated SpanAttributes [#4428](https://github.com/open-telemetry/opentelemetry-js/pull/4428) @JamieDanielson +* feat(sdk-metrics)!: remove MeterProvider.addMetricReader() in favor of constructor option [#4419](https://github.com/open-telemetry/opentelemetry-js/pull/4419) @pichlermarc +* feat(sdk-metrics)!: replace attributeKeys with custom processors option [#4532](https://github.com/open-telemetry/opentelemetry-js/pull/4532) @pichlermarc ### :rocket: (Enhancement) diff --git a/packages/sdk-metrics/src/index.ts b/packages/sdk-metrics/src/index.ts index 414766c099..e2f9ad2d0e 100644 --- a/packages/sdk-metrics/src/index.ts +++ b/packages/sdk-metrics/src/index.ts @@ -80,4 +80,10 @@ export { export { View, ViewOptions } from './view/View'; +export { + IAttributesProcessor, + createAllowListAttributesProcessor, + createDenyListAttributesProcessor, +} from './view/AttributesProcessor'; + export { TimeoutError } from './utils'; diff --git a/packages/sdk-metrics/src/state/AsyncMetricStorage.ts b/packages/sdk-metrics/src/state/AsyncMetricStorage.ts index 6bebafdc1f..b9871333de 100644 --- a/packages/sdk-metrics/src/state/AsyncMetricStorage.ts +++ b/packages/sdk-metrics/src/state/AsyncMetricStorage.ts @@ -17,7 +17,6 @@ import { HrTime } from '@opentelemetry/api'; import { Accumulation, Aggregator } from '../aggregator/types'; import { InstrumentDescriptor } from '../InstrumentDescriptor'; -import { AttributesProcessor } from '../view/AttributesProcessor'; import { MetricStorage } from './MetricStorage'; import { MetricData } from '../export/MetricData'; import { DeltaMetricProcessor } from './DeltaMetricProcessor'; @@ -26,6 +25,7 @@ import { Maybe } from '../utils'; import { MetricCollectorHandle } from './MetricCollector'; import { AttributeHashMap } from './HashMap'; import { AsyncWritableMetricStorage } from './WritableMetricStorage'; +import { IAttributesProcessor } from '../view/AttributesProcessor'; /** * Internal interface. @@ -42,7 +42,7 @@ export class AsyncMetricStorage> constructor( _instrumentDescriptor: InstrumentDescriptor, aggregator: Aggregator, - private _attributesProcessor: AttributesProcessor, + private _attributesProcessor: IAttributesProcessor, collectorHandles: MetricCollectorHandle[] ) { super(_instrumentDescriptor); diff --git a/packages/sdk-metrics/src/state/MeterSharedState.ts b/packages/sdk-metrics/src/state/MeterSharedState.ts index 2c0c1a5105..5e4c157b25 100644 --- a/packages/sdk-metrics/src/state/MeterSharedState.ts +++ b/packages/sdk-metrics/src/state/MeterSharedState.ts @@ -32,7 +32,10 @@ import { MultiMetricStorage } from './MultiWritableMetricStorage'; import { ObservableRegistry } from './ObservableRegistry'; import { SyncMetricStorage } from './SyncMetricStorage'; import { Accumulation, Aggregator } from '../aggregator/types'; -import { AttributesProcessor } from '../view/AttributesProcessor'; +import { + createNoopAttributesProcessor, + IAttributesProcessor, +} from '../view/AttributesProcessor'; import { MetricStorage } from './MetricStorage'; /** @@ -166,7 +169,7 @@ export class MeterSharedState { const storage = new MetricStorageType( descriptor, aggregator, - AttributesProcessor.Noop(), + createNoopAttributesProcessor(), [collector] ) as R; this.metricStorageRegistry.registerForCollector(collector, storage); @@ -189,7 +192,7 @@ interface MetricStorageConstructor { new ( instrumentDescriptor: InstrumentDescriptor, aggregator: Aggregator>, - attributesProcessor: AttributesProcessor, + attributesProcessor: IAttributesProcessor, collectors: MetricCollectorHandle[] ): MetricStorage; } diff --git a/packages/sdk-metrics/src/state/SyncMetricStorage.ts b/packages/sdk-metrics/src/state/SyncMetricStorage.ts index bb546e1271..0edfda4ddc 100644 --- a/packages/sdk-metrics/src/state/SyncMetricStorage.ts +++ b/packages/sdk-metrics/src/state/SyncMetricStorage.ts @@ -18,7 +18,7 @@ import { Context, HrTime, MetricAttributes } from '@opentelemetry/api'; import { WritableMetricStorage } from './WritableMetricStorage'; import { Accumulation, Aggregator } from '../aggregator/types'; import { InstrumentDescriptor } from '../InstrumentDescriptor'; -import { AttributesProcessor } from '../view/AttributesProcessor'; +import { IAttributesProcessor } from '../view/AttributesProcessor'; import { MetricStorage } from './MetricStorage'; import { MetricData } from '../export/MetricData'; import { DeltaMetricProcessor } from './DeltaMetricProcessor'; @@ -41,7 +41,7 @@ export class SyncMetricStorage> constructor( instrumentDescriptor: InstrumentDescriptor, aggregator: Aggregator, - private _attributesProcessor: AttributesProcessor, + private _attributesProcessor: IAttributesProcessor, collectorHandles: MetricCollectorHandle[] ) { super(instrumentDescriptor); diff --git a/packages/sdk-metrics/src/view/AttributesProcessor.ts b/packages/sdk-metrics/src/view/AttributesProcessor.ts index 53f2cddaff..d074385fef 100644 --- a/packages/sdk-metrics/src/view/AttributesProcessor.ts +++ b/packages/sdk-metrics/src/view/AttributesProcessor.ts @@ -14,14 +14,14 @@ * limitations under the License. */ -import { Context, MetricAttributes } from '@opentelemetry/api'; +import { Context, Attributes } from '@opentelemetry/api'; /** * The {@link AttributesProcessor} is responsible for customizing which * attribute(s) are to be reported as metrics dimension(s) and adding * additional dimension(s) from the {@link Context}. */ -export abstract class AttributesProcessor { +export interface IAttributesProcessor { /** * Process the metric instrument attributes. * @@ -29,33 +29,31 @@ export abstract class AttributesProcessor { * @param context The active context when the instrument is synchronous. * `undefined` otherwise. */ - abstract process( - incoming: MetricAttributes, - context?: Context - ): MetricAttributes; - - static Noop() { - return NOOP; - } + process: (incoming: Attributes, context?: Context) => Attributes; } -export class NoopAttributesProcessor extends AttributesProcessor { - process(incoming: MetricAttributes, _context?: Context) { +class NoopAttributesProcessor implements IAttributesProcessor { + process(incoming: Attributes, _context?: Context) { return incoming; } } -/** - * {@link AttributesProcessor} that filters by allowed attribute names and drops any names that are not in the - * allow list. - */ -export class FilteringAttributesProcessor extends AttributesProcessor { - constructor(private _allowedAttributeNames: string[]) { - super(); +class MultiAttributesProcessor implements IAttributesProcessor { + constructor(private readonly _processors: IAttributesProcessor[]) {} + process(incoming: Attributes, context?: Context): Attributes { + let filteredAttributes = incoming; + for (const processor of this._processors) { + filteredAttributes = processor.process(filteredAttributes, context); + } + return filteredAttributes; } +} + +class AllowListProcessor implements IAttributesProcessor { + constructor(private _allowedAttributeNames: string[]) {} - process(incoming: MetricAttributes, _context: Context): MetricAttributes { - const filteredAttributes: MetricAttributes = {}; + process(incoming: Attributes, _context?: Context): Attributes { + const filteredAttributes: Attributes = {}; Object.keys(incoming) .filter(attributeName => this._allowedAttributeNames.includes(attributeName) @@ -68,4 +66,62 @@ export class FilteringAttributesProcessor extends AttributesProcessor { } } +class DenyListProcessor implements IAttributesProcessor { + constructor(private _deniedAttributeNames: string[]) {} + + process(incoming: Attributes, _context?: Context): Attributes { + const filteredAttributes: Attributes = {}; + Object.keys(incoming) + .filter( + attributeName => !this._deniedAttributeNames.includes(attributeName) + ) + .forEach( + attributeName => + (filteredAttributes[attributeName] = incoming[attributeName]) + ); + return filteredAttributes; + } +} + +/** + * @internal + * + * Create an {@link IAttributesProcessor} that acts as a simple pass-through for attributes. + */ +export function createNoopAttributesProcessor(): IAttributesProcessor { + return NOOP; +} + +/** + * @internal + * + * Create an {@link IAttributesProcessor} that applies all processors from the provided list in order. + * + * @param processors Processors to apply in order. + */ +export function createMultiAttributesProcessor( + processors: IAttributesProcessor[] +): IAttributesProcessor { + return new MultiAttributesProcessor(processors); +} + +/** + * Create an {@link IAttributesProcessor} that filters by allowed attribute names and drops any names that are not in the + * allow list. + */ +export function createAllowListAttributesProcessor( + attributeAllowList: string[] +): IAttributesProcessor { + return new AllowListProcessor(attributeAllowList); +} + +/** + * Create an {@link IAttributesProcessor} that drops attributes based on the names provided in the deny list + */ +export function createDenyListAttributesProcessor( + attributeDenyList: string[] +): IAttributesProcessor { + return new DenyListProcessor(attributeDenyList); +} + const NOOP = new NoopAttributesProcessor(); diff --git a/packages/sdk-metrics/src/view/View.ts b/packages/sdk-metrics/src/view/View.ts index 1e8d4fb0e0..4d46c868ba 100644 --- a/packages/sdk-metrics/src/view/View.ts +++ b/packages/sdk-metrics/src/view/View.ts @@ -16,8 +16,9 @@ import { PatternPredicate } from './Predicate'; import { - AttributesProcessor, - FilteringAttributesProcessor, + createMultiAttributesProcessor, + createNoopAttributesProcessor, + IAttributesProcessor, } from './AttributesProcessor'; import { InstrumentSelector } from './InstrumentSelector'; import { MeterSelector } from './MeterSelector'; @@ -42,15 +43,18 @@ export type ViewOptions = { description?: string; /** * Alters the metric stream: - * If provided, the attributes that are not in the list will be ignored. + * If provided, the attributes will be modified as defined by the processors in the list. Processors are applied + * in the order they're provided. * If not provided, all attribute keys will be used by default. * * @example drops all attributes with top-level keys except for 'myAttr' and 'myOtherAttr' - * attributeKeys: ['myAttr', 'myOtherAttr'] + * attributesProcessors: [createAllowListProcessor(['myAttr', 'myOtherAttr'])] * @example drops all attributes - * attributeKeys: [] + * attributesProcessors: [createAllowListProcessor([])] + * @example allows all attributes except for 'myAttr' + * attributesProcessors: [createDenyListProcessor(['myAttr']] */ - attributeKeys?: string[]; + attributesProcessors?: IAttributesProcessor[]; /** * Alters the metric stream: * Alters the {@link Aggregation} of the metric stream. @@ -135,7 +139,7 @@ export class View { readonly name?: string; readonly description?: string; readonly aggregation: Aggregation; - readonly attributesProcessor: AttributesProcessor; + readonly attributesProcessor: IAttributesProcessor; readonly instrumentSelector: InstrumentSelector; readonly meterSelector: MeterSelector; @@ -157,9 +161,9 @@ export class View { * Alters the metric stream: * This will be used as the description of the metrics stream. * If not provided, the original Instrument description will be used by default. - * @param viewOptions.attributeKeys + * @param viewOptions.attributesProcessors * Alters the metric stream: - * If provided, the attributes that are not in the list will be ignored. + * If provided, the attributes will be modified as defined by the added processors. * If not provided, all attribute keys will be used by default. * @param viewOptions.aggregation * Alters the metric stream: @@ -210,13 +214,13 @@ export class View { ); } - // Create AttributesProcessor if attributeKeys are defined set. - if (viewOptions.attributeKeys != null) { - this.attributesProcessor = new FilteringAttributesProcessor( - viewOptions.attributeKeys + // Create multi-processor if attributesProcessors are defined. + if (viewOptions.attributesProcessors != null) { + this.attributesProcessor = createMultiAttributesProcessor( + viewOptions.attributesProcessors ); } else { - this.attributesProcessor = AttributesProcessor.Noop(); + this.attributesProcessor = createNoopAttributesProcessor(); } this.name = viewOptions.name; diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index 615fcffab3..364cd8779f 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -32,6 +32,7 @@ import { TestMetricReader } from './export/TestMetricReader'; import * as sinon from 'sinon'; import { View } from '../src/view/View'; import { Meter } from '../src/Meter'; +import { createAllowListAttributesProcessor } from '../src/view/AttributesProcessor'; describe('MeterProvider', () => { afterEach(() => { @@ -200,7 +201,7 @@ describe('MeterProvider', () => { ); }); - it('with attributeKeys should drop non-listed attributes', async () => { + it('with allowListProcessor should drop non-listed attributes', async () => { const reader = new TestMetricReader(); // Add view to drop all attributes except 'attrib1' @@ -208,7 +209,9 @@ describe('MeterProvider', () => { resource: defaultResource, views: [ new View({ - attributeKeys: ['attrib1'], + attributesProcessors: [ + createAllowListAttributesProcessor(['attrib1']), + ], instrumentName: 'non-renamed-instrument', }), ], diff --git a/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts b/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts index b4a5df1923..3e0f8970b7 100644 --- a/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts +++ b/packages/sdk-metrics/test/state/AsyncMetricStorage.test.ts @@ -21,7 +21,7 @@ import { AggregationTemporality } from '../../src/export/AggregationTemporality' import { DataPointType } from '../../src/export/MetricData'; import { MetricCollectorHandle } from '../../src/state/MetricCollector'; import { AsyncMetricStorage } from '../../src/state/AsyncMetricStorage'; -import { NoopAttributesProcessor } from '../../src/view/AttributesProcessor'; +import { createNoopAttributesProcessor } from '../../src/view/AttributesProcessor'; import { ObservableRegistry } from '../../src/state/ObservableRegistry'; import { assertMetricData, @@ -49,7 +49,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [deltaCollector] ); @@ -149,7 +149,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [deltaCollector] ); @@ -233,7 +233,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(false), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [deltaCollector] ); @@ -319,7 +319,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [cumulativeCollector] ); @@ -451,7 +451,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [cumulativeCollector] ); @@ -545,7 +545,7 @@ describe('AsyncMetricStorage', () => { const metricStorage = new AsyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(false), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [cumulativeCollector] ); diff --git a/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts b/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts index e2e0378a45..e50df8ef86 100644 --- a/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts +++ b/packages/sdk-metrics/test/state/SyncMetricStorage.test.ts @@ -22,7 +22,7 @@ import { AggregationTemporality } from '../../src/export/AggregationTemporality' import { DataPointType } from '../../src/export/MetricData'; import { MetricCollectorHandle } from '../../src/state/MetricCollector'; import { SyncMetricStorage } from '../../src/state/SyncMetricStorage'; -import { NoopAttributesProcessor } from '../../src/view/AttributesProcessor'; +import { createNoopAttributesProcessor } from '../../src/view/AttributesProcessor'; import { assertMetricData, assertDataPoint, @@ -45,7 +45,7 @@ describe('SyncMetricStorage', () => { const metricStorage = new SyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [] ); @@ -63,7 +63,7 @@ describe('SyncMetricStorage', () => { const metricStorage = new SyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [deltaCollector] ); @@ -101,7 +101,7 @@ describe('SyncMetricStorage', () => { const metricStorage = new SyncMetricStorage( defaultInstrumentDescriptor, new SumAggregator(true), - new NoopAttributesProcessor(), + createNoopAttributesProcessor(), [cumulativeCollector] ); metricStorage.record(1, {}, api.context.active(), [0, 0]); diff --git a/packages/sdk-metrics/test/view/AttributesProcessor.test.ts b/packages/sdk-metrics/test/view/AttributesProcessor.test.ts index 8d38efacdf..0bdcac9809 100644 --- a/packages/sdk-metrics/test/view/AttributesProcessor.test.ts +++ b/packages/sdk-metrics/test/view/AttributesProcessor.test.ts @@ -15,12 +15,19 @@ */ import * as assert from 'assert'; -import { context } from '@opentelemetry/api'; -import { NoopAttributesProcessor } from '../../src/view/AttributesProcessor'; -import { FilteringAttributesProcessor } from '../../src/view/AttributesProcessor'; +import { Attributes, context } from '@opentelemetry/api'; +import { + IAttributesProcessor, + createMultiAttributesProcessor, + createNoopAttributesProcessor, + createAllowListAttributesProcessor, + createDenyListAttributesProcessor, +} from '../../src/view/AttributesProcessor'; + +import sinon = require('sinon'); describe('NoopAttributesProcessor', () => { - const processor = new NoopAttributesProcessor(); + const processor = createNoopAttributesProcessor(); it('should return identical attributes on process', () => { assert.deepStrictEqual( @@ -32,14 +39,14 @@ describe('NoopAttributesProcessor', () => { }); }); -describe('FilteringAttributesProcessor', () => { +describe('AllowListProcessor', () => { it('should not add keys when attributes do not exist', () => { - const processor = new FilteringAttributesProcessor(['foo', 'bar']); + const processor = createAllowListAttributesProcessor(['foo', 'bar']); assert.deepStrictEqual(processor.process({}, context.active()), {}); }); it('should only keep allowed attributes', () => { - const processor = new FilteringAttributesProcessor(['foo', 'bar']); + const processor = createAllowListAttributesProcessor(['foo', 'bar']); assert.deepStrictEqual( processor.process( { @@ -56,3 +63,63 @@ describe('FilteringAttributesProcessor', () => { ); }); }); + +describe('DenyListProcessor', () => { + it('should drop denie attributes', () => { + const processor = createDenyListAttributesProcessor(['foo', 'bar']); + assert.deepStrictEqual( + processor.process( + { + foo: 'fooValue', + bar: 'barValue', + baz: 'bazValue', + }, + context.active() + ), + { + baz: 'bazValue', + } + ); + }); +}); + +describe('MultiAttributesProcessor', () => { + it('should apply in order', () => { + // arrange + const firstProcessorOutput: Attributes = { foo: 'firstProcessorFoo' }; + const secondProcessorOutput: Attributes = { + foo: 'secondProcessorFoo', + bar: 'secondProcessorBar', + }; + const firstMockProcessorStubs = { + process: sinon.stub().returns(firstProcessorOutput), + }; + const firstMockProcessor = firstMockProcessorStubs as IAttributesProcessor; + + const secondMockProcessorStubs = { + process: sinon.stub().returns(secondProcessorOutput), + }; + const secondMockProcessor = + secondMockProcessorStubs as IAttributesProcessor; + + const processor = createMultiAttributesProcessor([ + firstMockProcessor, + secondMockProcessor, + ]); + + // act + const input: Attributes = { foo: 'bar' }; + const result = processor.process(input, context.active()); + + // assert + firstMockProcessorStubs.process.calledOnceWithExactly( + input, + context.active() + ); + secondMockProcessorStubs.process.calledOnceWithExactly( + firstProcessorOutput, + context.active() + ); + assert.deepStrictEqual(result, secondProcessorOutput); + }); +}); diff --git a/packages/sdk-metrics/test/view/View.test.ts b/packages/sdk-metrics/test/view/View.test.ts index 921928cf1f..7328372390 100644 --- a/packages/sdk-metrics/test/view/View.test.ts +++ b/packages/sdk-metrics/test/view/View.test.ts @@ -15,7 +15,10 @@ */ import * as assert from 'assert'; -import { AttributesProcessor } from '../../src/view/AttributesProcessor'; +import { + createAllowListAttributesProcessor, + createNoopAttributesProcessor, +} from '../../src/view/AttributesProcessor'; import { View } from '../../src/view/View'; import { InstrumentType, @@ -33,7 +36,7 @@ describe('View', () => { assert.strictEqual(view.aggregation, Aggregation.Default()); assert.strictEqual( view.attributesProcessor, - AttributesProcessor.Noop() + createNoopAttributesProcessor() ); } { @@ -43,7 +46,7 @@ describe('View', () => { assert.strictEqual(view.aggregation, Aggregation.Default()); assert.strictEqual( view.attributesProcessor, - AttributesProcessor.Noop() + createNoopAttributesProcessor() ); } }); @@ -54,7 +57,12 @@ describe('View', () => { // would implicitly rename all instruments to 'name' assert.throws(() => new View({ name: 'name' })); // would implicitly drop all attribute keys on all instruments except 'key' - assert.throws(() => new View({ attributeKeys: ['key'] })); + assert.throws( + () => + new View({ + attributesProcessors: [createAllowListAttributesProcessor(['key'])], + }) + ); // would implicitly rename all instruments to description assert.throws(() => new View({ description: 'description' })); // would implicitly change all instruments to use histogram aggregation