-
Notifications
You must be signed in to change notification settings - Fork 821
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(tracing): support for truncatation of span attribute values #1624
feat(tracing): support for truncatation of span attribute values #1624
Conversation
d019e2b
to
fba9f13
Compare
@@ -47,6 +47,65 @@ export function isAttributeValue(val: unknown): val is AttributeValue { | |||
return isValidPrimitiveAttributeValue(val); | |||
} | |||
|
|||
export function truncateValueIfTooLong( | |||
value: AttributeValue, | |||
limit: number | undefined, |
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.
Please make limit
to be 3rd optional param.
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.
are you sure it makes sense for limit
to actually be optional in a function called truncateValueIfTooLong
? I've adjusted the type to number | null
, which seems more suitable
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.
yes as then you can call this function with 2 params only and it feels more natural
truncateValueIfTooLong('aaa', logger);
// instead of
truncateValueIfTooLong('aaa', undefined, logger);
// personally I would put logger as 1st param - it is always passed then value, then limit which is connected with value so it feels naturally to be after value.
truncateValueIfTooLong(logger, 'aaa', 123); // truncateValueIfTooLong(logger, 'aaa');
I see you already changed this to be a callback now
limit: number | undefined, | ||
logger: Logger | ||
): AttributeValue { | ||
if (limit == null) { |
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.
use ===
only but it this case if (!limit)
is enough
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.
maybe it makes sense to clarify in the specification what limit === 0
should mean? I don't think it's an unreasonable assumption to think that this means empty attribute values.
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.
I've addressed this by adding a minimum of 32 on the limit in the spec
} | ||
|
||
if (value.length > limit) { | ||
logger.warn('Value was truncated.'); |
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.
according to spec
To prevent excessive logging, the log should not be emitted more than once per span, attribute, event, or link.
here it will be emitted everytime
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.
I've adjusted the spec to say "the log MUST NOT be emitted more than once per item on which an attribute is set." and also added a check to only warn once per Span
.
if (Array.isArray(value)) { | ||
let accruedLength = 0; | ||
|
||
// roughly measure the size of a serialized array |
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.
if we add limit we should check the limit for all cases. The number might be
123131231231.321312312312
and if someone sets limit to 2 it should handle this case correctly too.
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.
Arrays truncation is now done using JSON.stringify
. Truncation of numbers seems difficult to me in general. So I've addressed this in the spec by "attribute value size limit MUST NOT be set to any number lower than 32". The only number-like type which could exceed 32 chars when stringified is bigint, and it's not allowed as attribute value.
} | ||
} | ||
|
||
if (accruedLength > limit) { |
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.
not sure why not stringify it first and skip all that counting. Sooner or later this value will be stringified anyway (either now or later in exporter) so counting value and stringify seems like extra overhead.
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.
Arrays truncation is now based on JSON.stringify
and backed by spec changes: "SDKs SHOULD choose any serialization protocol, which is performant and appropriate for the language and/or environment they are implemented in."
fba9f13
to
139bfcc
Compare
139bfcc
to
9f7c5f5
Compare
@jtmalinowski please don't force push for any PR when reviews has started as it is impossible to track the changes |
} | ||
|
||
if (limit < 32) { | ||
throw new Error('Value size limit cannot be lower than 32.'); |
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.
It should never throw any error, it can warn user but never throw.
with regards to limit I posted a reply on spec issue
@@ -47,6 +47,65 @@ export function isAttributeValue(val: unknown): val is AttributeValue { | |||
return isValidPrimitiveAttributeValue(val); | |||
} | |||
|
|||
export function truncateValueIfTooLong( | |||
value: AttributeValue, | |||
limit: number | undefined, |
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.
yes as then you can call this function with 2 params only and it feels more natural
truncateValueIfTooLong('aaa', logger);
// instead of
truncateValueIfTooLong('aaa', undefined, logger);
// personally I would put logger as 1st param - it is always passed then value, then limit which is connected with value so it feels naturally to be after value.
truncateValueIfTooLong(logger, 'aaa', 123); // truncateValueIfTooLong(logger, 'aaa');
I see you already changed this to be a callback now
@@ -112,7 +114,18 @@ export class Span implements api.Span, ReadableSpan { | |||
delete this.attributes[attributeKeyToDelete]; | |||
} | |||
} | |||
this.attributes[key] = value; | |||
|
|||
console.log('limit', this._traceParams.spanAttributeValueSizeLimit); |
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.
this should be removed ?
@@ -56,6 +57,7 @@ export class Span implements api.Span, ReadableSpan { | |||
endTime: api.HrTime = [0, 0]; | |||
private _ended = false; | |||
private _duration: api.HrTime = [-1, -1]; | |||
private _hasTruncated = false; |
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.
private _hasTruncated = false; | |
private _wasTruncated = false; |
? undefined | ||
: () => { | ||
this._hasTruncated = true; | ||
this._logger.warn(`Span attribute value truncated at key: ${key}.`); |
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.
this._logger.warn(`Span attribute value truncated at key: ${key}.`); | |
this._logger.warn(`Truncating span attribute value for key: ${key}.`); |
|
||
const configuredAttributeLimit = | ||
this._traceParams.spanAttributeValueSizeLimit || null; | ||
if (configuredAttributeLimit !== null && configuredAttributeLimit < 32) { |
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.
if (configuredAttributeLimit !== null && configuredAttributeLimit < 32) { | |
if (typeof configuredAttributeLimit === 'number' && configuredAttributeLimit < 32) { |
|
||
if (Array.isArray(value)) { | ||
// note: this is potentially incompatible with a given exporter | ||
const serialized = JSON.stringify(value); |
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.
this will be probably refactored to truncate each of the array element individually right ?
@obecny thank you for all the comments, let's try to close the spec PR first, because changes there mean changes here automatically, and then I'll address everything here |
This can be closed, @banothurameshnaik is addressing the last of the mentioned spec change in #2430. Tracking issue: #2403 |
…emetry#1624) * fix(fastify): Make sure consturctor patching works with esm * ref: Streamline fastify wrapping Align this with how other instrumentations wrap. * Revert "ref: Streamline fastify wrapping" This reverts commit cbffbc731fe81a559d355d3e6b2e726b8a3509ec. * fix linting --------- Co-authored-by: Marc Pichler <[email protected]> Co-authored-by: Amir Blum <[email protected]>
Which problem is this PR solving?
Short description of the changes