Skip to content

Commit

Permalink
feat(sdk-*)!: align merge resource behavior with spec (#5219)
Browse files Browse the repository at this point in the history
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
  • Loading branch information
david-luna and pichlermarc authored Dec 20, 2024
1 parent 8dc74e6 commit 90afa28
Show file tree
Hide file tree
Showing 13 changed files with 11 additions and 120 deletions.
8 changes: 1 addition & 7 deletions experimental/packages/opentelemetry-sdk-node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ export class NodeSDK {

private _resource: IResource;
private _resourceDetectors: Array<Detector | DetectorSync>;
private _mergeResourceWithDefaults: boolean;

private _autoDetectResources: boolean;

Expand Down Expand Up @@ -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 = [];
Expand Down Expand Up @@ -370,7 +367,6 @@ export class NodeSDK {
this._tracerProvider = new NodeTracerProvider({
...this._configuration,
resource: this._resource,
mergeResourceWithDefaults: this._mergeResourceWithDefaults,
spanProcessors,
});

Expand All @@ -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
Expand Down Expand Up @@ -417,7 +412,6 @@ export class NodeSDK {
resource: this._resource,
views: this._meterProviderConfig?.views ?? [],
readers: readers,
mergeResourceWithDefaults: this._mergeResourceWithDefaults,
});

this._meterProvider = meterProvider;
Expand Down
1 change: 0 additions & 1 deletion experimental/packages/opentelemetry-sdk-node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export interface NodeSDKConfiguration {
instrumentations: (Instrumentation | Instrumentation[])[];
resource: IResource;
resourceDetectors: Array<Detector | DetectorSync>;
mergeResourceWithDefaults?: boolean;
sampler: Sampler;
serviceName?: string;
/** @deprecated use spanProcessors instead*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
Expand Down
17 changes: 1 addition & 16 deletions experimental/packages/sdk-logs/src/LoggerProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>;
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,
Expand Down
1 change: 0 additions & 1 deletion experimental/packages/sdk-logs/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export function loadDefaultConfig() {
attributeCountLimit: getEnv().OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT,
},
includeTraceContext: true,
mergeResourceWithDefaults: true,
};
}

Expand Down
6 changes: 0 additions & 6 deletions experimental/packages/sdk-logs/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}

Expand Down
6 changes: 0 additions & 6 deletions packages/opentelemetry-sdk-trace-base/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down
26 changes: 1 addition & 25 deletions packages/sdk-metrics/src/MeterProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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) {
Expand Down
14 changes: 3 additions & 11 deletions packages/sdk-metrics/test/MeterProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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());
});
});

Expand Down

0 comments on commit 90afa28

Please sign in to comment.