diff --git a/CHANGELOG.md b/CHANGELOG.md index 66f17a5c5e6..602914230b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 * fix(opentelemetry-instrumentation): improve `_warnOnPreloadedModules` function not to show warning logs when the module is not marked as loaded [#6095](https://github.com/open-telemetry/opentelemetry-js/pull/6095) @rlj1202 * fix(sdk-trace-base): derive internal `SpanOptions` from API type to prevent drift [#6478](https://github.com/open-telemetry/opentelemetry-js/pull/6478) @overbalance +* fix(span): enforce `attributePerEventCountLimit`, `attributePerLinkCountLimit`, `linkCountLimit`, and `attributeValueLengthLimit` for event/link attributes [#6479](https://github.com/open-telemetry/opentelemetry-js/pull/6479) @overbalance ### :books: Documentation diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index ca48b502ac8..defff18a1a9 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -17,6 +17,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2 ### :bug: Bug Fixes * fix(opentelemetry-instrumentation): access `require` via `globalThis` to avoid webpack analysis [#6481](https://github.com/open-telemetry/opentelemetry-js/pull/6481) @overbalance +* fix(sdk-logs): fix inflated `droppedAttributesCount` when updating existing attribute keys [#6479](https://github.com/open-telemetry/opentelemetry-js/pull/6479) @overbalance ### :books: Documentation diff --git a/experimental/packages/sdk-logs/src/LogRecordImpl.ts b/experimental/packages/sdk-logs/src/LogRecordImpl.ts index 95671f87033..c71f2646023 100644 --- a/experimental/packages/sdk-logs/src/LogRecordImpl.ts +++ b/experimental/packages/sdk-logs/src/LogRecordImpl.ts @@ -35,7 +35,8 @@ export class LogRecordImpl implements ReadableLogRecord { private _severityNumber?: SeverityNumber; private _body?: LogBody; private _eventName?: string; - private totalAttributesCount: number = 0; + private _attributesCount: number = 0; + private _droppedAttributesCount: number = 0; private _isReadonly: boolean = false; private readonly _logRecordLimits: Required; @@ -81,7 +82,7 @@ export class LogRecordImpl implements ReadableLogRecord { } get droppedAttributesCount(): number { - return this.totalAttributesCount - Object.keys(this.attributes).length; + return this._droppedAttributesCount; } constructor( @@ -136,19 +137,25 @@ export class LogRecordImpl implements ReadableLogRecord { api.diag.warn(`Invalid attribute value set for key: ${key}`); return this; } - this.totalAttributesCount += 1; + const isNewKey = !Object.prototype.hasOwnProperty.call( + this.attributes, + key + ); if ( - Object.keys(this.attributes).length >= - this._logRecordLimits.attributeCountLimit && - !Object.prototype.hasOwnProperty.call(this.attributes, key) + isNewKey && + this._attributesCount >= this._logRecordLimits.attributeCountLimit ) { - // This logic is to create drop message at most once per LogRecord to prevent excessive logging. - if (this.droppedAttributesCount === 1) { + this._droppedAttributesCount++; + // Only warn once per LogRecord to avoid log spam + if (this._droppedAttributesCount === 1) { api.diag.warn('Dropping extra attributes.'); } return this; } this.attributes[key] = this._truncateToSize(value); + if (isNewKey) { + this._attributesCount++; + } return this; } diff --git a/experimental/packages/sdk-logs/test/common/LogRecord.test.ts b/experimental/packages/sdk-logs/test/common/LogRecord.test.ts index 2e1e6b039f8..649604a7f2d 100644 --- a/experimental/packages/sdk-logs/test/common/LogRecord.test.ts +++ b/experimental/packages/sdk-logs/test/common/LogRecord.test.ts @@ -301,6 +301,27 @@ describe('LogRecord', () => { sinon.assert.callCount(warnStub, 1); warnStub.restore(); }); + + it('should not increment droppedAttributesCount on key updates', () => { + const { logRecord } = setup({ attributeCountLimit: 2 }); + logRecord.setAttribute('key1', 'value1'); + logRecord.setAttribute('key2', 'value2'); + logRecord.setAttribute('key1', 'updated'); + assert.strictEqual(logRecord.droppedAttributesCount, 0); + assert.strictEqual(logRecord.attributes['key1'], 'updated'); + }); + + it('should correctly track droppedAttributesCount with key updates and dropped keys', () => { + const { logRecord } = setup({ attributeCountLimit: 2 }); + logRecord.setAttribute('key1', 'value1'); + logRecord.setAttribute('key2', 'value2'); + logRecord.setAttribute('key3', 'value3'); + logRecord.setAttribute('key1', 'updated'); + assert.strictEqual(logRecord.droppedAttributesCount, 1); + assert.strictEqual(logRecord.attributes['key1'], 'updated'); + assert.strictEqual(logRecord.attributes['key2'], 'value2'); + assert.strictEqual(logRecord.attributes['key3'], undefined); + }); }); describe('when "attributeValueLengthLimit" option defined', () => { diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index a1b900a245e..1344aa8e67a 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -115,13 +115,17 @@ export class SpanImpl implements Span { this._startTimeProvided = opts.startTime != null; this._spanLimits = opts.spanLimits; this._attributeValueLengthLimit = - this._spanLimits.attributeValueLengthLimit || 0; + this._spanLimits.attributeValueLengthLimit ?? 0; this._spanProcessor = opts.spanProcessor; this.name = opts.name; this.parentSpanContext = opts.parentSpanContext; this.kind = opts.kind; - this.links = opts.links || []; + if (opts.links) { + for (const link of opts.links) { + this.addLink(link); + } + } this.startTime = this._getTime(opts.startTime ?? now); this.resource = opts.resource; this.instrumentationScope = opts.scope; @@ -219,24 +223,83 @@ export class SpanImpl implements Span { attributesOrStartTime = undefined; } - const attributes = sanitizeAttributes(attributesOrStartTime); + const sanitized = sanitizeAttributes(attributesOrStartTime); + const { attributePerEventCountLimit } = this._spanLimits; + const entries = Object.entries(sanitized); + const attributes: Attributes = {}; + let droppedAttributesCount = 0; + + for (const [attr, attrVal] of entries) { + if ( + attributePerEventCountLimit !== undefined && + Object.keys(attributes).length >= attributePerEventCountLimit + ) { + droppedAttributesCount++; + continue; + } + attributes[attr] = this._truncateToSize(attrVal!); + } this.events.push({ name, attributes, time: this._getTime(timeStamp), - droppedAttributesCount: 0, + droppedAttributesCount, }); return this; } addLink(link: Link): this { - this.links.push(link); + if (this._isSpanEnded()) return this; + + const { linkCountLimit } = this._spanLimits; + + if (linkCountLimit === 0) { + this._droppedLinksCount++; + return this; + } + + if (linkCountLimit !== undefined && this.links.length >= linkCountLimit) { + if (this._droppedLinksCount === 0) { + diag.debug('Dropping extra links.'); + } + this.links.shift(); + this._droppedLinksCount++; + } + + const { attributePerLinkCountLimit } = this._spanLimits; + const sanitized = sanitizeAttributes(link.attributes); + const entries = Object.entries(sanitized); + const attributes: Attributes = {}; + let droppedAttributesCount = 0; + + for (const [attr, attrVal] of entries) { + if ( + attributePerLinkCountLimit !== undefined && + Object.keys(attributes).length >= attributePerLinkCountLimit + ) { + droppedAttributesCount++; + continue; + } + attributes[attr] = this._truncateToSize(attrVal!); + } + + const processedLink: Link = { context: link.context }; + if (Object.keys(attributes).length > 0) { + processedLink.attributes = attributes; + } + if (droppedAttributesCount > 0) { + processedLink.droppedAttributesCount = droppedAttributesCount; + } + + this.links.push(processedLink); return this; } addLinks(links: Link[]): this { - this.links.push(...links); + for (const link of links) { + this.addLink(link); + } return this; } @@ -296,6 +359,11 @@ export class SpanImpl implements Span { `Dropped ${this._droppedEventsCount} events because eventCountLimit reached` ); } + if (this._droppedLinksCount > 0) { + diag.warn( + `Dropped ${this._droppedLinksCount} links because linkCountLimit reached` + ); + } if (this._spanProcessor.onEnding) { this._spanProcessor.onEnding(this); } diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index c8258466995..c3d68f4ab0e 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -943,6 +943,247 @@ describe('Span', () => { assert.strictEqual(span.events.length, 0); }); + it('should enforce attributePerEventCountLimit', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + attributePerEventCountLimit: 2, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addEvent('testEvent', { + attr1: 'value1', + attr2: 'value2', + attr3: 'value3', + attr4: 'value4', + attr5: 'value5', + }); + span.end(); + + assert.strictEqual(span.events.length, 1); + assert.strictEqual(Object.keys(span.events[0].attributes!).length, 2); + assert.strictEqual(span.events[0].droppedAttributesCount, 3); + }); + + it('should truncate event attribute values', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + attributeValueLengthLimit: 5, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addEvent('testEvent', { + longAttr: 'abcdefghij', + shortAttr: 'abc', + }); + span.end(); + + assert.strictEqual(span.events[0].attributes!['longAttr'], 'abcde'); + assert.strictEqual(span.events[0].attributes!['shortAttr'], 'abc'); + }); + + it('should enforce attributePerLinkCountLimit', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + attributePerLinkCountLimit: 2, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addLink({ + context: linkContext, + attributes: { + attr1: 'value1', + attr2: 'value2', + attr3: 'value3', + attr4: 'value4', + attr5: 'value5', + }, + }); + span.end(); + + assert.strictEqual(span.links.length, 1); + assert.strictEqual(Object.keys(span.links[0].attributes!).length, 2); + assert.strictEqual(span.links[0].droppedAttributesCount, 3); + }); + + it('should truncate link attribute values', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + attributeValueLengthLimit: 5, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addLink({ + context: linkContext, + attributes: { + longAttr: 'abcdefghij', + shortAttr: 'abc', + }, + }); + span.end(); + + assert.strictEqual(span.links[0].attributes!['longAttr'], 'abcde'); + assert.strictEqual(span.links[0].attributes!['shortAttr'], 'abc'); + }); + + it('should enforce linkCountLimit with FIFO', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + linkCountLimit: 3, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + for (let i = 0; i < 5; i++) { + span.addLink({ + context: linkContext, + attributes: { index: i }, + }); + } + span.end(); + + assert.strictEqual(span.links.length, 3); + assert.strictEqual(span.links[0].attributes!['index'], 2); + assert.strictEqual(span.links[1].attributes!['index'], 3); + assert.strictEqual(span.links[2].attributes!['index'], 4); + assert.strictEqual(span.droppedLinksCount, 2); + }); + + it('should enforce linkCountLimit via constructor', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + linkCountLimit: 2, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + links: [ + { context: linkContext, attributes: { index: 0 } }, + { context: linkContext, attributes: { index: 1 } }, + { context: linkContext, attributes: { index: 2 } }, + { context: linkContext, attributes: { index: 3 } }, + { context: linkContext, attributes: { index: 4 } }, + ], + }); + span.end(); + + assert.strictEqual(span.links.length, 2); + assert.strictEqual(span.links[0].attributes!['index'], 3); + assert.strictEqual(span.links[1].attributes!['index'], 4); + assert.strictEqual(span.droppedLinksCount, 3); + }); + + it('should drop all links when linkCountLimit is 0', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + linkCountLimit: 0, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addLink({ context: linkContext }); + span.addLink({ context: linkContext }); + span.end(); + + assert.strictEqual(span.links.length, 0); + assert.strictEqual(span.droppedLinksCount, 2); + }); + + it('should enforce linkCountLimit via addLinks', () => { + const tracer = new BasicTracerProvider({ + spanLimits: { + linkCountLimit: 2, + }, + }).getTracer('default') as Tracer; + + const span = new SpanImpl({ + scope: tracer.instrumentationScope, + resource: tracer['_resource'], + context: ROOT_CONTEXT, + spanContext, + name, + kind: SpanKind.CLIENT, + spanLimits: tracer.getSpanLimits(), + spanProcessor: tracer['_spanProcessor'], + }); + span.addLinks([ + { context: linkContext, attributes: { index: 0 } }, + { context: linkContext, attributes: { index: 1 } }, + { context: linkContext, attributes: { index: 2 } }, + ]); + span.end(); + + assert.strictEqual(span.links.length, 2); + assert.strictEqual(span.links[0].attributes!['index'], 1); + assert.strictEqual(span.links[1].attributes!['index'], 2); + assert.strictEqual(span.droppedLinksCount, 1); + }); + describe('setStatus', () => { it('should set an error status', () => { const span = new SpanImpl({