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

feat(opentelemetry-sdk-trace-base): implemented option to limit length of values of attributes #2418

Merged
2 changes: 2 additions & 0 deletions packages/opentelemetry-core/src/utils/environment.ts
Expand Up @@ -28,6 +28,7 @@ const ENVIRONMENT_NUMBERS_KEYS = [
'OTEL_BSP_MAX_EXPORT_BATCH_SIZE',
'OTEL_BSP_MAX_QUEUE_SIZE',
'OTEL_BSP_SCHEDULE_DELAY',
'OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT',
'OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT',
'OTEL_SPAN_EVENT_COUNT_LIMIT',
'OTEL_SPAN_LINK_COUNT_LIMIT',
Expand Down Expand Up @@ -117,6 +118,7 @@ export const DEFAULT_ENVIRONMENT: Required<ENVIRONMENT> = {
OTEL_PROPAGATORS: ['tracecontext', 'baggage'],
OTEL_RESOURCE_ATTRIBUTES: '',
OTEL_SERVICE_NAME: '',
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: Infinity,
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: 128,
OTEL_SPAN_EVENT_COUNT_LIMIT: 128,
OTEL_SPAN_LINK_COUNT_LIMIT: 128,
Expand Down
2 changes: 2 additions & 0 deletions packages/opentelemetry-core/test/utils/environment.test.ts
Expand Up @@ -84,6 +84,7 @@ describe('environment', () => {
OTEL_LOG_LEVEL: 'ERROR',
OTEL_NO_PATCH_MODULES: 'a,b,c',
OTEL_RESOURCE_ATTRIBUTES: '<attrs>',
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT: 100,
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: 10,
OTEL_SPAN_EVENT_COUNT_LIMIT: 20,
OTEL_SPAN_LINK_COUNT_LIMIT: 30,
Expand All @@ -93,6 +94,7 @@ describe('environment', () => {
const env = getEnv();
assert.deepStrictEqual(env.OTEL_NO_PATCH_MODULES, ['a', 'b', 'c']);
assert.strictEqual(env.OTEL_LOG_LEVEL, DiagLogLevel.ERROR);
assert.strictEqual(env.OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT, 100);
assert.strictEqual(env.OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT, 10);
assert.strictEqual(env.OTEL_SPAN_EVENT_COUNT_LIMIT, 20);
assert.strictEqual(env.OTEL_SPAN_LINK_COUNT_LIMIT, 30);
Expand Down
60 changes: 59 additions & 1 deletion packages/opentelemetry-sdk-trace-base/src/Span.ts
Expand Up @@ -105,7 +105,7 @@ export class Span implements api.Span, ReadableSpan {
) {
return this;
}
this.attributes[key] = value;
this.attributes[key] = this._truncateToSize(value);
return this;
}

Expand Down Expand Up @@ -235,4 +235,62 @@ export class Span implements api.Span, ReadableSpan {
}
return this._ended;
}

// Utility function to truncate given value within size
// for value type of string, will truncate to given limit
// for type of non-string, will return same value
private _truncateToLimitUtil(value: string, limit: number): string {
if (typeof value != 'string' || value.length <= limit) {
return value;
}
return value.substr(0, limit);
}

// Check whether given value is array of strings or not
private _isArrayOfStrings(value: unknown): boolean {
if (!Array.isArray(value)) {
return false
}
return value.every(val => typeof val == 'string')
}

/**
* If the given attribute value is of type string and has more characters than given {@code attributeValueLengthLimit} then
* return string with trucated to {@code attributeValueLengthLimit} characters
*
* If the given attribute value is array of strings then
* return new array of strings with each element truncated to {@code attributeValueLengthLimit} characters
*
* Otherwise return same Attribute {@code value}
*
* @param value Attribute value
* @returns truncated attribute value if required, otherwise same value
*/
private _truncateToSize(value?: SpanAttributeValue): SpanAttributeValue | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private _truncateToSize(value?: SpanAttributeValue): SpanAttributeValue | undefined {
private _truncateToSize(value: SpanAttributeValue): SpanAttributeValue {

const limit = this._spanLimits.attributeValueLengthLimit!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the ! assertion needed here?

// Check limit
if (typeof limit != 'number' || limit <= 0) {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use strict typing

Suggested change
if (typeof limit != 'number' || limit <= 0) {
if (typeof limit !== 'number' || limit <= 0) {

or even do

if (!(limit > 0))

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OT: Why is lint not complaining about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good question, maybe some rule is missing, as some time ago we updated lint, wondering if that was on purpose then or it was missed.

api.diag.warn(`Attribute value limit must be positive, got ${limit}`);
dyladan marked this conversation as resolved.
Show resolved Hide resolved
return value;
}

// Check type of values
// Allowed types are string, array of strings
if (value == null || (typeof value != 'string' && !Array.isArray(value))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will already fall through to the return at the end

Suggested change
if (value == null || (typeof value != 'string' && !Array.isArray(value))) {
if (value == null) {

return value;
}

// String
if (typeof value == 'string') {
return this._truncateToLimitUtil(value, limit);
}

// Array of strings
if (this._isArrayOfStrings(value)) {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@obecny obecny Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec doesn't allow to have array of different type. Imagine the situation when you have 1k of attributes and only one is a number and the rest will still be string. From a user perspective I would assume that all 999 attributes should be truncated and the rest not. Having this both things in mind I would get rid of function _isArrayOfStrings and would simply do.

return (value as []).map(val => {
  typeof val  === 'string' ? this._truncateToLimitUtil(val, limit)) : val;
}

This way code will not break and the attributes that can be limited will be limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the same way at the first glance.
but later added array of strings check since spec here specially says apply these truncate only for strings and array of strings otherwise ignore.

Let me change back as you mentioned.

return value.map(val => this._truncateToLimitUtil(val as string, limit));
}

// Other types, no need to apply value length limit
return value;
}
}
1 change: 1 addition & 0 deletions packages/opentelemetry-sdk-trace-base/src/config.ts
Expand Up @@ -38,6 +38,7 @@ export const DEFAULT_CONFIG = {
sampler: buildSamplerFromEnv(env),
forceFlushTimeoutMillis: 30000,
spanLimits: {
attributeValueLengthLimit: getEnv().OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT,
attributeCountLimit: getEnv().OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT,
linkCountLimit: getEnv().OTEL_SPAN_LINK_COUNT_LIMIT,
eventCountLimit: getEnv().OTEL_SPAN_EVENT_COUNT_LIMIT,
Expand Down
2 changes: 2 additions & 0 deletions packages/opentelemetry-sdk-trace-base/src/types.ts
Expand Up @@ -63,6 +63,8 @@ export interface SDKRegistrationConfig {

/** Global configuration of trace service */
export interface SpanLimits {
/** attributeValueLengthLimit is maximum allowed attribute value size */
attributeValueLengthLimit?: number;
/** attributeCountLimit is number of attributes per span */
attributeCountLimit?: number;
/** linkCountLimit is number of links per span */
Expand Down
Expand Up @@ -79,6 +79,7 @@ describe('BasicTracerProvider', () => {
it('should construct an instance with default span limits', () => {
const tracer = new BasicTracerProvider({}).getTracer('default');
assert.deepStrictEqual(tracer.getSpanLimits(), {
attributeValueLengthLimit: Infinity,
attributeCountLimit: 128,
eventCountLimit: 128,
linkCountLimit: 128,
Expand All @@ -92,19 +93,35 @@ describe('BasicTracerProvider', () => {
},
}).getTracer('default');
assert.deepStrictEqual(tracer.getSpanLimits(), {
attributeValueLengthLimit: Infinity,
attributeCountLimit: 100,
eventCountLimit: 128,
linkCountLimit: 128,
});
});

it('should construct an instance with customized attributeValueLengthLimit span limits', () => {
Copy link
Member

@obecny obecny Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having difficulties with understanding this description and what is happening here, consider this

Suggested change
it('should construct an instance with customized attributeValueLengthLimit span limits', () => {
// earlier
describe('"spanLimits"', ()=> {
...
describe('when "attributeValueLengthLimit" is defined', ()=> {
it('should truncate value which length exceeds this limit', () => {
});
});
describe('when "attributeCountLimit" is defined', ()=> {
it('should remove / drop all remaining values after the number of values exceeds this limit, () => {
});
}
// etc. etc. just use more natural language to describe it with 'when' -> 'then' approach
....

WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. It's adds better readability also 👍

I followed existing code test cases pattern since its my first contribution to open source

I will make changes here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obecny I made test cases as you mentioned.

const tracer = new BasicTracerProvider({
spanLimits: {
attributeValueLengthLimit: 10,
},
}).getTracer('default');
assert.deepStrictEqual(tracer.getSpanLimits(), {
attributeValueLengthLimit: 10,
attributeCountLimit: 128,
eventCountLimit: 128,
linkCountLimit: 128,
});
});

it('should construct an instance with customized eventCountLimit span limits', () => {
const tracer = new BasicTracerProvider({
spanLimits: {
eventCountLimit: 300,
},
}).getTracer('default');
assert.deepStrictEqual(tracer.getSpanLimits(), {
attributeValueLengthLimit: Infinity,
attributeCountLimit: 128,
eventCountLimit: 300,
linkCountLimit: 128,
Expand All @@ -118,6 +135,7 @@ describe('BasicTracerProvider', () => {
},
}).getTracer('default');
assert.deepStrictEqual(tracer.getSpanLimits(), {
attributeValueLengthLimit: Infinity,
attributeCountLimit: 128,
eventCountLimit: 128,
linkCountLimit: 10,
Expand Down
34 changes: 34 additions & 0 deletions packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts
Expand Up @@ -37,6 +37,7 @@ const performanceTimeOrigin = hrTime();
describe('Span', () => {
const tracer = new BasicTracerProvider({
spanLimits: {
attributeValueLengthLimit: 100,
attributeCountLimit: 100,
eventCountLimit: 100,
},
Expand Down Expand Up @@ -261,6 +262,39 @@ describe('Span', () => {
});
});

it('should truncate values more than attribute value length limit', () => {
const tracer = new BasicTracerProvider({
spanLimits: {
// Setting attribute value length limit
attributeValueLengthLimit: 5,
attributeCountLimit: 100,
eventCountLimit: 100,
},
}).getTracer('default');

const span = new Span(
tracer,
ROOT_CONTEXT,
name,
spanContext,
SpanKind.CLIENT
);

span.setAttribute('attr-with-more-length', 'abcdefgh');
span.setAttribute('attr-with-less-length', 'abc');
span.setAttribute('attr-empty-string', '');
span.setAttribute('attr-non-string', true);
span.setAttribute('attr-array-of-strings', ['abcdefgh', 'abc', 'abcde', '']);

assert.deepStrictEqual(span.attributes, {
'attr-with-more-length': 'abcde',
'attr-with-less-length': 'abc',
'attr-empty-string': '',
'attr-non-string': true,
'attr-array-of-strings': ['abcde', 'abc', 'abcde', ''],
});
});

it('should set attributes', () => {
const span = new Span(
tracer,
Expand Down