feat(tracing): support for truncatation of span attribute values #1624

jtmalinowski · 2020-10-26T21:06:42Z

Which problem is this PR solving?

implementation for Span attribute value truncatation as per incoming spec update Add an option to limit length of values of attributes and metric values opentelemetry-specification#1130

Short description of the changes

new utility function for truncating attribute values
truncating function applied when setting attributes on Span

obecny · 2020-10-26T22:23:37Z

packages/opentelemetry-core/src/common/attributes.ts

@@ -47,6 +47,65 @@ export function isAttributeValue(val: unknown): val is AttributeValue {
  return isValidPrimitiveAttributeValue(val);
 }

+export function truncateValueIfTooLong(
+  value: AttributeValue,
+  limit: number | undefined,


Please make limit to be 3rd optional param.

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

~~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

obecny · 2020-10-26T22:37:10Z

packages/opentelemetry-core/src/common/attributes.ts

+  limit: number | undefined,
+  logger: Logger
+): AttributeValue {
+  if (limit == null) {


use === only but it this case if (!limit) is enough

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.

I've addressed this by adding a minimum of 32 on the limit in the spec

packages/opentelemetry-core/src/common/attributes.ts

obecny · 2020-10-26T23:01:14Z

packages/opentelemetry-core/src/common/attributes.ts

+  }
+
+  if (value.length > limit) {
+    logger.warn('Value was truncated.');


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

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.

obecny · 2020-10-26T23:10:47Z

packages/opentelemetry-core/src/common/attributes.ts

+  if (Array.isArray(value)) {
+    let accruedLength = 0;
+
+    // roughly measure the size of a serialized array


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.

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.

obecny · 2020-10-26T23:14:26Z

packages/opentelemetry-core/src/common/attributes.ts

+      }
+    }
+
+    if (accruedLength > limit) {


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.

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."

obecny · 2020-11-03T23:13:55Z

@jtmalinowski please don't force push for any PR when reviews has started as it is impossible to track the changes

obecny · 2020-11-03T23:15:41Z

packages/opentelemetry-core/src/common/attributes.ts

+  }
+
+  if (limit < 32) {
+    throw new Error('Value size limit cannot be lower than 32.');


It should never throw any error, it can warn user but never throw.
with regards to limit I posted a reply on spec issue

obecny · 2020-11-03T23:39:13Z

packages/opentelemetry-core/src/common/attributes.ts

@@ -47,6 +47,65 @@ export function isAttributeValue(val: unknown): val is AttributeValue {
  return isValidPrimitiveAttributeValue(val);
 }

+export function truncateValueIfTooLong(
+  value: AttributeValue,
+  limit: number | undefined,


~~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

obecny · 2020-11-03T23:42:29Z

packages/opentelemetry-tracing/src/Span.ts

@@ -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);


this should be removed ?

obecny · 2020-11-03T23:55:03Z

packages/opentelemetry-tracing/src/Span.ts

@@ -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;


Suggested change

private _hasTruncated = false;

private _wasTruncated = false;

obecny · 2020-11-03T23:59:24Z

packages/opentelemetry-tracing/src/Span.ts

+        ? undefined
+        : () => {
+            this._hasTruncated = true;
+            this._logger.warn(`Span attribute value truncated at key: ${key}.`);


Suggested change

this._logger.warn(`Span attribute value truncated at key: ${key}.`);

this._logger.warn(`Truncating span attribute value for key: ${key}.`);

obecny · 2020-11-04T00:01:27Z

packages/opentelemetry-tracing/src/Tracer.ts

+
+    const configuredAttributeLimit =
+      this._traceParams.spanAttributeValueSizeLimit || null;
+    if (configuredAttributeLimit !== null && configuredAttributeLimit < 32) {


Suggested change

if (configuredAttributeLimit !== null && configuredAttributeLimit < 32) {

if (typeof configuredAttributeLimit === 'number' && configuredAttributeLimit < 32) {

obecny · 2020-11-04T00:07:21Z

packages/opentelemetry-core/src/common/attributes.ts

+
+  if (Array.isArray(value)) {
+    // note: this is potentially incompatible with a given exporter
+    const serialized = JSON.stringify(value);


this will be probably refactored to truncate each of the array element individually right ?

jtmalinowski · 2020-11-06T13:15:23Z

@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

rauno56 · 2021-09-07T17:34:02Z

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 <marc.pichler@dynatrace.com> Co-authored-by: Amir Blum <amirgiraffe@gmail.com>

jtmalinowski requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners October 26, 2020 21:06

jtmalinowski force-pushed the feature/span-attr-truncation branch from d019e2b to fba9f13 Compare October 26, 2020 21:12

obecny added the release:after-ga label Oct 26, 2020

obecny reviewed Oct 26, 2020

View reviewed changes

jtmalinowski force-pushed the feature/span-attr-truncation branch from fba9f13 to 139bfcc Compare October 28, 2020 13:20

feat(tracing): support for truncation of span attribute values

9f7c5f5

jtmalinowski force-pushed the feature/span-attr-truncation branch from 139bfcc to 9f7c5f5 Compare October 28, 2020 17:18

obecny reviewed Nov 3, 2020

View reviewed changes

obecny reviewed Nov 4, 2020

View reviewed changes

Base automatically changed from master to main January 25, 2021 19:26

dyladan requested review from Flarna and johnbley as code owners January 25, 2021 19:26

rauno56 closed this Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): support for truncatation of span attribute values #1624

feat(tracing): support for truncatation of span attribute values #1624

jtmalinowski commented Oct 26, 2020

obecny Oct 26, 2020

jtmalinowski Oct 28, 2020

obecny Nov 3, 2020 •

edited

obecny Oct 26, 2020

jtmalinowski Oct 27, 2020

jtmalinowski Oct 28, 2020

obecny Oct 26, 2020

jtmalinowski Oct 28, 2020

obecny Oct 26, 2020

jtmalinowski Oct 28, 2020

obecny Oct 26, 2020

jtmalinowski Oct 28, 2020

obecny commented Nov 3, 2020

obecny Nov 3, 2020

obecny Nov 3, 2020 •

edited

obecny Nov 3, 2020

obecny Nov 3, 2020

obecny Nov 3, 2020

obecny Nov 4, 2020

obecny Nov 4, 2020

jtmalinowski commented Nov 6, 2020

rauno56 commented Sep 7, 2021

	private _hasTruncated = false;
	private _wasTruncated = false;

	this._logger.warn(`Span attribute value truncated at key: ${key}.`);
	this._logger.warn(`Truncating span attribute value for key: ${key}.`);

	if (configuredAttributeLimit !== null && configuredAttributeLimit < 32) {
	if (typeof configuredAttributeLimit === 'number' && configuredAttributeLimit < 32) {

feat(tracing): support for truncatation of span attribute values #1624

feat(tracing): support for truncatation of span attribute values #1624

Conversation

jtmalinowski commented Oct 26, 2020

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Nov 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny commented Nov 3, 2020

Choose a reason for hiding this comment

obecny Nov 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtmalinowski commented Nov 6, 2020

rauno56 commented Sep 7, 2021

obecny Nov 3, 2020 •

edited

obecny Nov 3, 2020 •

edited