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

banothurameshnaik · 2021-08-18T04:31:43Z

Which problem is this PR solving?

Fixes: Spec: implement an option to limit length of values of attributes and metric values #2403

Short description of the changes

Implemented option to limit length of values of attributes
Ticket Spec: implement an option to limit length of values of attributes and metric values #2403 says about same changes for metrics also, but this PR solves only for attributes.

…th of values of attributes Signed-off-by: Banothu Ramesh Naik <[email protected]>

linux-foundation-easycla · 2021-08-18T04:31:46Z

The committers are authorized under a signed CLA.

✅ Banothu Ramesh Naik (c6b4981)

codecov · 2021-08-18T14:10:15Z

Codecov Report

Merging #2418 (835e9c5) into main (5bb9fcc) will increase coverage by 0.53%.
The diff coverage is 100.00%.

❗ Current head 835e9c5 differs from pull request most recent head 97e619b. Consider uploading reports for the commit 97e619b to get more accurate results

@@            Coverage Diff             @@
##             main    #2418      +/-   ##
==========================================
+ Coverage   92.14%   92.68%   +0.53%     
==========================================
  Files         120      137      +17     
  Lines        3998     4990     +992     
  Branches      844     1054     +210     
==========================================
+ Hits         3684     4625     +941     
- Misses        314      365      +51

Impacted Files	Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts	`95.83% <ø> (ø)`
...ackages/opentelemetry-sdk-trace-base/src/config.ts	`86.84% <ø> (ø)`
packages/opentelemetry-sdk-trace-base/src/Span.ts	`100.00% <100.00%> (+1.92%)`	⬆️
...emetry-api-metrics/src/platform/node/globalThis.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...entelemetry-sdk-trace-web/src/WebTracerProvider.ts	`100.00% <0.00%> (ø)`
...ry-exporter-collector/src/CollectorExporterBase.ts	`92.15% <0.00%> (ø)`
...lemetry-exporter-collector/src/transformMetrics.ts	`95.71% <0.00%> (ø)`
...emetry-instrumentation-xml-http-request/src/xhr.ts	`97.58% <0.00%> (ø)`
...mentation-xml-http-request/src/enums/EventNames.ts	`100.00% <0.00%> (ø)`
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	`96.91% <0.00%> (ø)`
... and 12 more

packages/opentelemetry-sdk-trace-base/src/Span.ts

dyladan · 2021-08-18T14:12:19Z

packages/opentelemetry-sdk-trace-base/src/Span.ts

+   * @returns truncated attribute value if required, otherwise same value
+   */
+  private _truncateToSize(value?: SpanAttributeValue): SpanAttributeValue | undefined {
+    const limit = this._spanLimits.attributeValueLengthLimit!


Why is the ! assertion needed here?

packages/opentelemetry-sdk-trace-base/src/Span.ts

dyladan · 2021-08-18T14:14:02Z

packages/opentelemetry-sdk-trace-base/src/Span.ts

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


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) {

…ovements Signed-off-by: Banothu Ramesh Naik <[email protected]>

packages/opentelemetry-sdk-trace-base/src/Span.ts

obecny · 2021-08-18T20:41:23Z

packages/opentelemetry-sdk-trace-base/src/Span.ts

+    }
+
+    // Array of strings
+    if (this._isArrayOfStrings(value)) {


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.

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.

obecny · 2021-08-18T20:44:33Z

packages/opentelemetry-sdk-trace-base/src/Span.ts

+    const limit = this._spanLimits.attributeValueLengthLimit;
+    // Check limit
+    // undefined limit means do not truncate
+    if (typeof limit != 'number' || limit <= 0) {


please use strict typing

Suggested change

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

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

or even do

if (!(limit > 0))

?

OT: Why is lint not complaining about this?

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.

obecny · 2021-08-18T20:46:15Z

packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts

        attributeCountLimit: 100,
        eventCountLimit: 128,
        linkCountLimit: 128,
      });
    });

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


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 ?

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

@obecny I made test cases as you mentioned.

Signed-off-by: Banothu Ramesh Naik <[email protected]>

obecny

lgtm, thx for changing tests it is much readable now :)

obecny · 2021-08-19T21:19:26Z

packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts

-    span.setAttribute('array<string>', ['str1', 'str2']);
-    span.setAttribute('array<number>', [1, 2]);
-    span.setAttribute('array<bool>', [true, false]);
+      it('should able to overwrite attributes', () => {


?

Suggested change

it('should able to overwrite attributes', () => {

it('should be able to overwrite attributes', () => {

obecny · 2021-08-19T21:19:56Z

packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts

-      SpanKind.CLIENT
-    );
+  describe('setAttributes', () => {
+    it('should able to set multiple attributes', () => {


Suggested change

it('should able to set multiple attributes', () => {

it('should be able to set multiple attributes', () => {

Signed-off-by: Banothu Ramesh Naik <[email protected]>

banothurameshnaik · 2021-08-21T07:32:41Z

@dyladan @Flarna
I made changes as you mentioned in comments.

Can you please have a look.

Flarna · 2021-08-21T08:38:50Z

packages/opentelemetry-sdk-trace-base/src/Span.ts

+   * @param value Attribute value
+   * @returns truncated attribute value if required, otherwise same value
+   */
+  private _truncateToSize(value?: SpanAttributeValue): SpanAttributeValue | undefined {


Suggested change

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

private _truncateToSize(value: SpanAttributeValue): SpanAttributeValue {

packages/opentelemetry-sdk-trace-base/src/Span.ts

Signed-off-by: Banothu Ramesh Naik <[email protected]>

chore(opentelemetry-sdk-trace-base): implemented option to limit leng…

c6b4981

…th of values of attributes Signed-off-by: Banothu Ramesh Naik <[email protected]>

banothurameshnaik requested a review from a team August 18, 2021 04:31

Merge branch 'main' into feature/span-attribute-value-length-limit

84582c6

banothurameshnaik mentioned this pull request Aug 18, 2021

Spec: implement an option to limit length of values of attributes and metric values #2403

Closed

dyladan reviewed Aug 18, 2021

View reviewed changes

fix(opentelemetry-sdk-trace-base): pr suggested changes and code impr…

af8dbe0

…ovements Signed-off-by: Banothu Ramesh Naik <[email protected]>

dyladan reviewed Aug 18, 2021

View reviewed changes

packages/opentelemetry-sdk-trace-base/src/Span.ts Show resolved Hide resolved

dyladan changed the title ~~chore(opentelemetry-sdk-trace-base): implemented option to limit length of values of attributes~~ feat(opentelemetry-sdk-trace-base): implemented option to limit length of values of attributes Aug 18, 2021

obecny reviewed Aug 18, 2021

View reviewed changes

banothurameshnaik added 3 commits August 19, 2021 07:47

Merge branch 'main' into feature/span-attribute-value-length-limit

1e6cf3c

fix(opentelemetry-sdk-trace-base): pr suggested changes

99e0eb5

Signed-off-by: Banothu Ramesh Naik <[email protected]>

fix(opentelemetry-sdk-trace-base): pr suggested changes on test cases

a327e4a

Signed-off-by: Banothu Ramesh Naik <[email protected]>

obecny approved these changes Aug 19, 2021

View reviewed changes

fix(opentelemetry-sdk-trace-base): pr suggested changes

4815460

Signed-off-by: Banothu Ramesh Naik <[email protected]>

Flarna reviewed Aug 21, 2021

View reviewed changes

packages/opentelemetry-sdk-trace-base/src/Span.ts Show resolved Hide resolved

Flarna reviewed Aug 21, 2021

View reviewed changes

packages/opentelemetry-sdk-trace-base/src/Span.ts Outdated Show resolved Hide resolved

Flarna approved these changes Aug 21, 2021

View reviewed changes

fix(opentelemetry-sdk-trace-base): pr suggested changes

48c81ff

Signed-off-by: Banothu Ramesh Naik <[email protected]>

Flarna approved these changes Aug 23, 2021

View reviewed changes

rauno56 approved these changes Aug 23, 2021

View reviewed changes

Merge branch 'main' into feature/span-attribute-value-length-limit

97e619b

dyladan approved these changes Aug 23, 2021

View reviewed changes

obecny added the enhancement New feature or request label Aug 23, 2021

obecny merged commit 78a78c0 into open-telemetry:main Aug 23, 2021

dyladan mentioned this pull request Sep 13, 2021

chore: v1.0.0 proposal #2468

Merged

dyladan mentioned this pull request Sep 30, 2021

chore(experimental): v0.26.0 proposal #2513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

banothurameshnaik commented Aug 18, 2021 •

edited

Loading

linux-foundation-easycla bot commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 18, 2021 •

edited

Loading

dyladan Aug 18, 2021

dyladan Aug 18, 2021

obecny Aug 18, 2021 •

edited

Loading

banothurameshnaik Aug 19, 2021

obecny Aug 18, 2021

Flarna Aug 18, 2021

obecny Aug 19, 2021

obecny Aug 18, 2021 •

edited

Loading

banothurameshnaik Aug 19, 2021

banothurameshnaik Aug 19, 2021

obecny left a comment

obecny Aug 19, 2021

obecny Aug 19, 2021

banothurameshnaik commented Aug 21, 2021

Flarna Aug 21, 2021

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

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

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

	it('should able to overwrite attributes', () => {
	it('should be able to overwrite attributes', () => {

	it('should able to set multiple attributes', () => {
	it('should be able to set multiple attributes', () => {

	private _truncateToSize(value?: SpanAttributeValue): SpanAttributeValue \| undefined {
	private _truncateToSize(value: SpanAttributeValue): SpanAttributeValue {

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

Conversation

banothurameshnaik commented Aug 18, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Aug 18, 2021 • edited Loading

codecov bot commented Aug 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Aug 18, 2021 • edited Loading

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 Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banothurameshnaik commented Aug 21, 2021

Choose a reason for hiding this comment

banothurameshnaik commented Aug 18, 2021 •

edited

Loading

linux-foundation-easycla bot commented Aug 18, 2021 •

edited

Loading

codecov bot commented Aug 18, 2021 •

edited

Loading

obecny Aug 18, 2021 •

edited

Loading

obecny Aug 18, 2021 •

edited

Loading