Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(sdk-trace-base): make resource private and remove getActiveSpanProcessor API #5192

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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
Loading