From 90afa2850c0690f7a18ecc511c04927a3183490b Mon Sep 17 00:00:00 2001 From: David Luna Date: Fri, 20 Dec 2024 14:38:22 +0100 Subject: [PATCH] feat(sdk-*)!: align merge resource behavior with spec (#5219) Co-authored-by: Marc Pichler --- .../opentelemetry-sdk-node/src/sdk.ts | 8 +----- .../opentelemetry-sdk-node/src/types.ts | 1 - .../opentelemetry-sdk-node/test/sdk.test.ts | 2 +- .../packages/sdk-logs/src/LoggerProvider.ts | 17 +----------- experimental/packages/sdk-logs/src/config.ts | 1 - experimental/packages/sdk-logs/src/types.ts | 6 ----- .../test/common/LoggerProvider.test.ts | 26 +------------------ .../src/BasicTracerProvider.ts | 6 +---- .../src/config.ts | 1 - .../opentelemetry-sdk-trace-base/src/types.ts | 6 ----- .../test/common/BasicTracerProvider.test.ts | 17 ++---------- packages/sdk-metrics/src/MeterProvider.ts | 26 +------------------ .../sdk-metrics/test/MeterProvider.test.ts | 14 +++------- 13 files changed, 11 insertions(+), 120 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index 52ffb8c51c2..1da7d0e071d 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -210,7 +210,6 @@ export class NodeSDK { private _resource: IResource; private _resourceDetectors: Array; - private _mergeResourceWithDefaults: boolean; private _autoDetectResources: boolean; @@ -245,9 +244,7 @@ export class NodeSDK { this._configuration = configuration; - this._resource = configuration.resource ?? new Resource({}); - this._mergeResourceWithDefaults = - configuration.mergeResourceWithDefaults ?? true; + this._resource = configuration.resource ?? Resource.default(); this._autoDetectResources = configuration.autoDetectResources ?? true; if (!this._autoDetectResources) { this._resourceDetectors = []; @@ -370,7 +367,6 @@ export class NodeSDK { this._tracerProvider = new NodeTracerProvider({ ...this._configuration, resource: this._resource, - mergeResourceWithDefaults: this._mergeResourceWithDefaults, spanProcessors, }); @@ -388,7 +384,6 @@ export class NodeSDK { if (this._loggerProviderConfig) { const loggerProvider = new LoggerProvider({ resource: this._resource, - mergeResourceWithDefaults: this._mergeResourceWithDefaults, }); for (const logRecordProcessor of this._loggerProviderConfig @@ -417,7 +412,6 @@ export class NodeSDK { resource: this._resource, views: this._meterProviderConfig?.views ?? [], readers: readers, - mergeResourceWithDefaults: this._mergeResourceWithDefaults, }); this._meterProvider = meterProvider; diff --git a/experimental/packages/opentelemetry-sdk-node/src/types.ts b/experimental/packages/opentelemetry-sdk-node/src/types.ts index 4a143692149..0683a3d886f 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/types.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/types.ts @@ -40,7 +40,6 @@ export interface NodeSDKConfiguration { instrumentations: (Instrumentation | Instrumentation[])[]; resource: IResource; resourceDetectors: Array; - mergeResourceWithDefaults?: boolean; sampler: Sampler; serviceName?: string; /** @deprecated use spanProcessors instead*/ diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 87c7e0f8491..3c22a986e52 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -942,7 +942,7 @@ describe('Node SDK', () => { const resource = sdk['_resource']; await resource.waitForAsyncAttributes?.(); - assert.deepStrictEqual(resource, Resource.empty()); + assert.deepStrictEqual(resource, Resource.default()); await sdk.shutdown(); }); }); diff --git a/experimental/packages/sdk-logs/src/LoggerProvider.ts b/experimental/packages/sdk-logs/src/LoggerProvider.ts index bdb402db933..575f178178e 100644 --- a/experimental/packages/sdk-logs/src/LoggerProvider.ts +++ b/experimental/packages/sdk-logs/src/LoggerProvider.ts @@ -28,28 +28,13 @@ import { LoggerProviderSharedState } from './internal/LoggerProviderSharedState' export const DEFAULT_LOGGER_NAME = 'unknown'; -function prepareResource( - mergeWithDefaults: boolean, - providedResource: Resource | undefined -) { - const resource = providedResource ?? Resource.empty(); - - if (mergeWithDefaults) { - return Resource.default().merge(resource); - } - return resource; -} - export class LoggerProvider implements logsAPI.LoggerProvider { private _shutdownOnce: BindOnceFuture; private readonly _sharedState: LoggerProviderSharedState; constructor(config: LoggerProviderConfig = {}) { const mergedConfig = merge({}, loadDefaultConfig(), config); - const resource = prepareResource( - mergedConfig.mergeResourceWithDefaults, - config.resource - ); + const resource = config.resource ?? Resource.default(); this._sharedState = new LoggerProviderSharedState( resource, mergedConfig.forceFlushTimeoutMillis, diff --git a/experimental/packages/sdk-logs/src/config.ts b/experimental/packages/sdk-logs/src/config.ts index b61163b2fa8..91b2c3e4884 100644 --- a/experimental/packages/sdk-logs/src/config.ts +++ b/experimental/packages/sdk-logs/src/config.ts @@ -31,7 +31,6 @@ export function loadDefaultConfig() { attributeCountLimit: getEnv().OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT, }, includeTraceContext: true, - mergeResourceWithDefaults: true, }; } diff --git a/experimental/packages/sdk-logs/src/types.ts b/experimental/packages/sdk-logs/src/types.ts index de316a0e34a..27aefa540fe 100644 --- a/experimental/packages/sdk-logs/src/types.ts +++ b/experimental/packages/sdk-logs/src/types.ts @@ -28,12 +28,6 @@ export interface LoggerProviderConfig { /** Log Record Limits*/ logRecordLimits?: LogRecordLimits; - - /** - * Merge resource with {@link Resource.default()}? - * Default: {@code true} - */ - mergeResourceWithDefaults?: boolean; } export interface LogRecordLimits { diff --git a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts index b2499fce7e9..be1a7321515 100644 --- a/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts +++ b/experimental/packages/sdk-logs/test/common/LoggerProvider.test.ts @@ -63,39 +63,15 @@ describe('LoggerProvider', () => { assert.deepStrictEqual(resource, Resource.default()); }); - it('should fallback to default resource attrs', () => { - const passedInResource = new Resource({ foo: 'bar' }); - const provider = new LoggerProvider({ resource: passedInResource }); - const { resource } = provider['_sharedState']; - assert.deepStrictEqual( - resource, - Resource.default().merge(passedInResource) - ); - }); - - it('should not merge with default resource attrs when flag is set to false', function () { + it('should not have default resource if passed', function () { const passedInResource = new Resource({ foo: 'bar' }); const provider = new LoggerProvider({ resource: passedInResource, - mergeResourceWithDefaults: false, }); const { resource } = provider['_sharedState']; assert.deepStrictEqual(resource, passedInResource); }); - it('should merge with default resource attrs when flag is set to true', function () { - const passedInResource = new Resource({ foo: 'bar' }); - const provider = new LoggerProvider({ - resource: passedInResource, - mergeResourceWithDefaults: true, - }); - const { resource } = provider['_sharedState']; - assert.deepStrictEqual( - resource, - Resource.default().merge(passedInResource) - ); - }); - it('should have default forceFlushTimeoutMillis if not pass', () => { const provider = new LoggerProvider(); const sharedState = provider['_sharedState']; diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index 042e98beba2..542d0872854 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -78,11 +78,7 @@ export class BasicTracerProvider implements TracerProvider { loadDefaultConfig(), reconfigureLimits(config) ); - this._resource = mergedConfig.resource ?? Resource.empty(); - - if (mergedConfig.mergeResourceWithDefaults) { - this._resource = Resource.default().merge(this._resource); - } + this._resource = mergedConfig.resource ?? Resource.default(); this._config = Object.assign({}, mergedConfig, { resource: this._resource, diff --git a/packages/opentelemetry-sdk-trace-base/src/config.ts b/packages/opentelemetry-sdk-trace-base/src/config.ts index bf767a61670..a53ca6ab670 100644 --- a/packages/opentelemetry-sdk-trace-base/src/config.ts +++ b/packages/opentelemetry-sdk-trace-base/src/config.ts @@ -53,7 +53,6 @@ export function loadDefaultConfig() { env.OTEL_SPAN_ATTRIBUTE_PER_EVENT_COUNT_LIMIT, attributePerLinkCountLimit: env.OTEL_SPAN_ATTRIBUTE_PER_LINK_COUNT_LIMIT, }, - mergeResourceWithDefaults: true, }; } diff --git a/packages/opentelemetry-sdk-trace-base/src/types.ts b/packages/opentelemetry-sdk-trace-base/src/types.ts index f351c0ce072..9c3bc222a8c 100644 --- a/packages/opentelemetry-sdk-trace-base/src/types.ts +++ b/packages/opentelemetry-sdk-trace-base/src/types.ts @@ -35,12 +35,6 @@ export interface TracerConfig { /** Span Limits */ spanLimits?: SpanLimits; - /** - * Merge resource with {@link Resource.default()}? - * Default: {@code true} - **/ - mergeResourceWithDefaults?: boolean; - /** Resource associated with trace telemetry */ resource?: IResource; 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 62395128f9f..a52e8775df7 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -932,25 +932,12 @@ describe('BasicTracerProvider', () => { assert.deepStrictEqual(tracerProvider['_resource'], Resource.default()); }); - it('should not merge with defaults when flag is set to false', function () { - const expectedResource = new Resource({ foo: 'bar' }); - const tracerProvider = new BasicTracerProvider({ - mergeResourceWithDefaults: false, - resource: expectedResource, - }); - assert.deepStrictEqual(tracerProvider['_resource'], expectedResource); - }); - - it('should merge with defaults when flag is set to true', function () { + it('should use not use the default if resource passed', function () { const providedResource = new Resource({ foo: 'bar' }); const tracerProvider = new BasicTracerProvider({ - mergeResourceWithDefaults: true, resource: providedResource, }); - assert.deepStrictEqual( - tracerProvider['_resource'], - Resource.default().merge(providedResource) - ); + assert.deepStrictEqual(tracerProvider['_resource'], providedResource); }); }); diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts index fa370da46b5..0d8d3e13b4d 100644 --- a/packages/sdk-metrics/src/MeterProvider.ts +++ b/packages/sdk-metrics/src/MeterProvider.ts @@ -36,27 +36,6 @@ export interface MeterProviderOptions { resource?: IResource; views?: ViewOptions[]; readers?: MetricReader[]; - /** - * Merge resource with {@link Resource.default()}? - * Default: {@code true} - */ - mergeResourceWithDefaults?: boolean; -} - -/** - * @param mergeWithDefaults - * @param providedResource - */ -function prepareResource( - mergeWithDefaults: boolean, - providedResource: Resource | undefined -) { - const resource = providedResource ?? Resource.empty(); - - if (mergeWithDefaults) { - return Resource.default().merge(resource); - } - return resource; } /** @@ -68,10 +47,7 @@ export class MeterProvider implements IMeterProvider { constructor(options?: MeterProviderOptions) { this._sharedState = new MeterProviderSharedState( - prepareResource( - options?.mergeResourceWithDefaults ?? true, - options?.resource - ) + options?.resource ?? Resource.default() ); if (options?.views != null && options.views.length > 0) { for (const viewOption of options.views) { diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index 05cd76e1e23..51df6018854 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -68,14 +68,13 @@ describe('MeterProvider', () => { assert.deepStrictEqual(resourceMetrics.resource, Resource.default()); }); - it('should not merge with defaults when flag is set to false', async function () { + it('should use the resource passed in constructor', async function () { const reader = new TestMetricReader(); const expectedResource = new Resource({ foo: 'bar' }); const meterProvider = new MeterProvider({ readers: [reader], resource: expectedResource, - mergeResourceWithDefaults: false, }); // Create meter and instrument, otherwise nothing will export @@ -88,14 +87,10 @@ describe('MeterProvider', () => { assert.deepStrictEqual(resourceMetrics.resource, expectedResource); }); - it('should merge with defaults when flag is set to true', async function () { + it('should use default resource if not passed in constructor', async function () { const reader = new TestMetricReader(); - const providedResource = new Resource({ foo: 'bar' }); - const meterProvider = new MeterProvider({ readers: [reader], - resource: providedResource, - mergeResourceWithDefaults: true, }); // Create meter and instrument, otherwise nothing will export @@ -105,10 +100,7 @@ describe('MeterProvider', () => { // Perform collection. const { resourceMetrics } = await reader.collect(); - assert.deepStrictEqual( - resourceMetrics.resource, - Resource.default().merge(providedResource) - ); + assert.deepStrictEqual(resourceMetrics.resource, Resource.default()); }); });