-
Notifications
You must be signed in to change notification settings - Fork 825
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
feat(opentelemetry-sdk-trace-base): implemented option to limit length of values of attributes #2418
Changes from 2 commits
c6b4981
84582c6
af8dbe0
1e6cf3c
99e0eb5
a327e4a
4815460
48c81ff
97e619b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
} | ||||||
|
||||||
|
@@ -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 { | ||||||
const limit = this._spanLimits.attributeValueLengthLimit! | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the |
||||||
// Check limit | ||||||
if (typeof limit != 'number' || limit <= 0) { | ||||||
dyladan marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use strict typing
Suggested change
or even do
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OT: Why is lint not complaining about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This way code will not break and the attributes that can be limited will be limited. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I implemented the same way at the first glance. 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; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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, | ||||||||||||||||||||||||||||||||||
|
@@ -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', () => { | ||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
WDYT ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||||||||||||||||||||||||||||||||
|
@@ -118,6 +135,7 @@ describe('BasicTracerProvider', () => { | |||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||
}).getTracer('default'); | ||||||||||||||||||||||||||||||||||
assert.deepStrictEqual(tracer.getSpanLimits(), { | ||||||||||||||||||||||||||||||||||
attributeValueLengthLimit: Infinity, | ||||||||||||||||||||||||||||||||||
attributeCountLimit: 128, | ||||||||||||||||||||||||||||||||||
eventCountLimit: 128, | ||||||||||||||||||||||||||||||||||
linkCountLimit: 10, | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.