Skip to content

Commit

Permalink
refactor(sdk-trace-base): make resource private and remove getActiveS…
Browse files Browse the repository at this point in the history
…panProcessor API (#5192)
  • Loading branch information
david-luna authored Nov 25, 2024
1 parent f3a6310 commit c28abac
Show file tree
Hide file tree
Showing 11 changed files with 616 additions and 434 deletions.
1 change: 1 addition & 0 deletions CHANGELOG_NEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* feat(sdk-metrics)!: drop `View` and `Aggregation` in favor of `ViewOptions` and `AggregationOption` [#4931](https://github.com/open-telemetry/opentelemetry-js/pull/4931) @pichlermarc
* refactor(sdk-trace-base)!: remove `new Span` constructor in favor of `Tracer.startSpan` API [#5048](https://github.com/open-telemetry/opentelemetry-js/pull/5048) @david-luna
* refactor(sdk-trace-base)!: remove `BasicTracerProvider.addSpanProcessor` API in favor of constructor options. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna
* refactor(sdk-trace-base)!: make `resource` property private in `BasicTracerProvider` and remove `getActiveSpanProcessor` API. [#5192](https://github.com/open-telemetry/opentelemetry-js/pull/5192) @david-luna

### :rocket: (Enhancement)

Expand Down
4 changes: 2 additions & 2 deletions experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ describe('Node SDK', () => {

assert.ok(nodeTracerProvider instanceof NodeTracerProvider);

const spanProcessor = nodeTracerProvider.getActiveSpanProcessor() as any;
const spanProcessor = nodeTracerProvider['activeSpanProcessor'] as any;

assert(
spanProcessor.constructor.name === 'MultiSpanProcessor',
Expand Down Expand Up @@ -1117,7 +1117,7 @@ describe('setup exporter from env', () => {

assert(tracerProvider instanceof NodeTracerProvider);

const activeSpanProcessor = tracerProvider.getActiveSpanProcessor();
const activeSpanProcessor = tracerProvider['activeSpanProcessor'];

assert(activeSpanProcessor.constructor.name === 'MultiSpanProcessor');

Expand Down
15 changes: 6 additions & 9 deletions packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,24 @@ export class BasicTracerProvider implements TracerProvider {

private readonly _config: TracerConfig;
private readonly _tracers: Map<string, Tracer> = new Map();
private readonly _resource: IResource;

private activeSpanProcessor: MultiSpanProcessor;
readonly resource: IResource;

constructor(config: TracerConfig = {}) {
const mergedConfig = merge(
{},
loadDefaultConfig(),
reconfigureLimits(config)
);
this.resource = mergedConfig.resource ?? Resource.empty();
this._resource = mergedConfig.resource ?? Resource.empty();

if (mergedConfig.mergeResourceWithDefaults) {
this.resource = Resource.default().merge(this.resource);
this._resource = Resource.default().merge(this._resource);
}

this._config = Object.assign({}, mergedConfig, {
resource: this.resource,
resource: this._resource,
});

const spanProcessors: SpanProcessor[] = [];
Expand Down Expand Up @@ -116,7 +116,8 @@ export class BasicTracerProvider implements TracerProvider {
new Tracer(
{ name, version, schemaUrl: options?.schemaUrl },
this._config,
this
this._resource,
this.activeSpanProcessor
)
);
}
Expand All @@ -125,10 +126,6 @@ export class BasicTracerProvider implements TracerProvider {
return this._tracers.get(key)!;
}

getActiveSpanProcessor(): SpanProcessor {
return this.activeSpanProcessor;
}

/**
* Register this TracerProvider for use with the OpenTelemetry API.
* Undefined values may be replaced with defaults, and
Expand Down
64 changes: 33 additions & 31 deletions packages/opentelemetry-sdk-trace-base/src/Span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ import { ReadableSpan } from './export/ReadableSpan';
import { ExceptionEventName } from './enums';
import { SpanProcessor } from './SpanProcessor';
import { TimedEvent } from './TimedEvent';
import { Tracer } from './Tracer';
import { SpanLimits } from './types';

/**
Expand All @@ -61,6 +60,21 @@ import { SpanLimits } from './types';
*/
export type Span = APISpan & ReadableSpan;

interface SpanOptions {
resource: IResource;
scope: InstrumentationLibrary;
context: Context;
spanContext: SpanContext;
name: string;
kind: SpanKind;
parentSpanId?: string;
links?: Link[];
startTime?: TimeInput;
attributes?: Attributes;
spanLimits: SpanLimits;
spanProcessor: SpanProcessor;
}

/**
* This class represents a span.
*/
Expand Down Expand Up @@ -99,44 +113,32 @@ export class SpanImpl implements Span {
/**
* Constructs a new SpanImpl instance.
*/
constructor(
parentTracer: Tracer,
context: Context,
spanName: string,
spanContext: SpanContext,
kind: SpanKind,
parentSpanId?: string,
links: Link[] = [],
startTime?: TimeInput,
_deprecatedClock?: unknown, // keeping this argument even though it is unused to ensure backwards compatibility
attributes?: Attributes
) {
this.name = spanName;
this._spanContext = spanContext;
this.parentSpanId = parentSpanId;
this.kind = kind;
this.links = links;

constructor(opts: SpanOptions) {
const now = Date.now();

this._spanContext = opts.spanContext;
this._performanceStartTime = otperformance.now();
this._performanceOffset =
now - (this._performanceStartTime + getTimeOrigin());
this._startTimeProvided = startTime != null;

this.startTime = this._getTime(startTime ?? now);

this.resource = parentTracer.resource;
this.instrumentationLibrary = parentTracer.instrumentationLibrary;
this._spanLimits = parentTracer.getSpanLimits();
this._startTimeProvided = opts.startTime != null;
this._spanLimits = opts.spanLimits;
this._attributeValueLengthLimit =
this._spanLimits.attributeValueLengthLimit || 0;

if (attributes != null) {
this.setAttributes(attributes);
this._spanProcessor = opts.spanProcessor;

this.name = opts.name;
this.parentSpanId = opts.parentSpanId;
this.kind = opts.kind;
this.links = opts.links || [];
this.startTime = this._getTime(opts.startTime ?? now);
this.resource = opts.resource;
this.instrumentationLibrary = opts.scope;

if (opts.attributes != null) {
this.setAttributes(opts.attributes);
}

this._spanProcessor = parentTracer.getActiveSpanProcessor();
this._spanProcessor.onStart(this, context);
this._spanProcessor.onStart(this, opts.context);
}

spanContext(): SpanContext {
Expand Down
37 changes: 19 additions & 18 deletions packages/opentelemetry-sdk-trace-base/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@ import {
sanitizeAttributes,
isTracingSuppressed,
} from '@opentelemetry/core';
import { IResource } from '@opentelemetry/resources';
import { BasicTracerProvider } from './BasicTracerProvider';
import { SpanImpl } from './Span';
import { GeneralLimits, SpanLimits, TracerConfig } from './types';
import { mergeConfig } from './utility';
import { SpanProcessor } from './SpanProcessor';
import { Sampler } from './Sampler';
import { IdGenerator } from './IdGenerator';
import { RandomIdGenerator } from './platform';
import { IResource } from '@opentelemetry/resources';

/**
* This class represents a basic tracer.
Expand All @@ -38,23 +37,27 @@ export class Tracer implements api.Tracer {
private readonly _generalLimits: GeneralLimits;
private readonly _spanLimits: SpanLimits;
private readonly _idGenerator: IdGenerator;
readonly resource: IResource;
readonly instrumentationLibrary: InstrumentationLibrary;

private readonly _resource: IResource;
private readonly _spanProcessor: SpanProcessor;

/**
* Constructs a new Tracer instance.
*/
constructor(
instrumentationLibrary: InstrumentationLibrary,
config: TracerConfig,
private _tracerProvider: BasicTracerProvider
resource: IResource,
spanProcessor: SpanProcessor
) {
const localConfig = mergeConfig(config);
this._sampler = localConfig.sampler;
this._generalLimits = localConfig.generalLimits;
this._spanLimits = localConfig.spanLimits;
this._idGenerator = config.idGenerator || new RandomIdGenerator();
this.resource = _tracerProvider.resource;
this._resource = resource;
this._spanProcessor = spanProcessor;
this.instrumentationLibrary = instrumentationLibrary;
}

Expand Down Expand Up @@ -138,18 +141,20 @@ export class Tracer implements api.Tracer {
Object.assign(attributes, samplingResult.attributes)
);

const span = new SpanImpl(
this,
const span = new SpanImpl({
resource: this._resource,
scope: this.instrumentationLibrary,
context,
name,
spanContext,
spanKind,
parentSpanId,
name,
kind: spanKind,
links,
options.startTime,
undefined,
initAttributes
);
parentSpanId,
attributes: initAttributes,
startTime: options.startTime,
spanProcessor: this._spanProcessor,
spanLimits: this._spanLimits,
});
return span;
}

Expand Down Expand Up @@ -250,8 +255,4 @@ export class Tracer implements api.Tracer {
getSpanLimits(): SpanLimits {
return this._spanLimits;
}

getActiveSpanProcessor(): SpanProcessor {
return this._tracerProvider.getActiveSpanProcessor();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,8 @@ describe('BasicTracerProvider', () => {
const tracerProvider = new BasicTracerProvider();
const tracer = tracerProvider.getTracer('default');
const span = tracer.startSpan('my-span') as Span;
assert.strictEqual(tracer.resource, tracerProvider.resource);
assert.strictEqual(span.resource, tracerProvider.resource);
assert.strictEqual(tracer['_resource'], tracerProvider['_resource']);
assert.strictEqual(span.resource, tracerProvider['_resource']);
});

it('should start a span with name and options', () => {
Expand Down Expand Up @@ -929,7 +929,7 @@ describe('BasicTracerProvider', () => {
describe('.resource', () => {
it('should use the default resource when no resource is provided', function () {
const tracerProvider = new BasicTracerProvider();
assert.deepStrictEqual(tracerProvider.resource, Resource.default());
assert.deepStrictEqual(tracerProvider['_resource'], Resource.default());
});

it('should not merge with defaults when flag is set to false', function () {
Expand All @@ -938,7 +938,7 @@ describe('BasicTracerProvider', () => {
mergeResourceWithDefaults: false,
resource: expectedResource,
});
assert.deepStrictEqual(tracerProvider.resource, expectedResource);
assert.deepStrictEqual(tracerProvider['_resource'], expectedResource);
});

it('should merge with defaults when flag is set to true', function () {
Expand All @@ -948,7 +948,7 @@ describe('BasicTracerProvider', () => {
resource: providedResource,
});
assert.deepStrictEqual(
tracerProvider.resource,
tracerProvider['_resource'],
Resource.default().merge(providedResource)
);
});
Expand All @@ -958,7 +958,7 @@ describe('BasicTracerProvider', () => {
it('should trigger shutdown when manually invoked', () => {
const tracerProvider = new BasicTracerProvider();
const shutdownStub = sinon.stub(
tracerProvider.getActiveSpanProcessor(),
tracerProvider['activeSpanProcessor'],
'shutdown'
);
tracerProvider.shutdown();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('MultiSpanProcessor', () => {
assert.strictEqual(processor1.spans.length, 0);
span.end();
assert.strictEqual(processor1.spans.length, 1);
tracerProvider.getActiveSpanProcessor().shutdown();
tracerProvider['activeSpanProcessor'].shutdown();
});

it('should handle two span processor', async () => {
Expand All @@ -81,9 +81,6 @@ describe('MultiSpanProcessor', () => {
assert.strictEqual(processor1.spans.length, 1);
assert.strictEqual(processor1.spans.length, processor2.spans.length);

// await tracerProvider.getActiveSpanProcessor().shutdown();
// assert.strictEqual(processor1.spans.length, 0);
// assert.strictEqual(processor1.spans.length, processor2.spans.length);
tracerProvider.shutdown().then(() => {
assert.strictEqual(processor1.spans.length, 0);
assert.strictEqual(processor1.spans.length, processor2.spans.length);
Expand Down
Loading

0 comments on commit c28abac

Please sign in to comment.