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

fix(sdk-trace-base): fix spanLimits attribute length/count to consider env values #3068

Merged

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Jun 29, 2022

Which problem is this PR solving?

OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT and OTEL_ATTRIBUTE_COUNT_LIMIT env values were being ignored when re-configuring span limits.

Fixes #3049

Short description of the changes

Updated the reconfigureLimits function to check additional items before updating spanLimits.attributeValueLengthLimit and spanLimits.attributeCountLimit property.

BEFORE
Before, reconfigureLimits wasn't considering if the user set the general attribute value length limit (OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT) via env - it was only checking if user set that value programmatically. Same issue with the attribute count limit OTEL_ATTRIBUTE_COUNT_LIMIT.

NOW
Now it checks if the user did not set the spanLimits.attributeValueLengthLimit programmatically AND if OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT env value is still set to the default value (ex. Infinity) BUT if the user set the generalLimits.attributeValueLengthLimit programmatically OR user set the OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT via env, then reconfigure the spanLimits.attributeValueLengthLimit value to either use the generalLimits.attributeValueLengthLimit set programmatically or use the OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env value.

Note: I had to wrap the old DEFAULT_CONFIG object into a function because those envs value (parsed in DEFAULT_CONFIG) were being parsed before the tests ran so it caused the new tests to fail. I added a comment about that above that function just in case someone had questions about it but I can remove the comment if needed.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

…ider valued defined in env

Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #3068 (86fc4f6) into main (776656b) will decrease coverage by 0.44%.
The diff coverage is 88.00%.

❗ Current head 86fc4f6 differs from pull request most recent head b959b72. Consider uploading reports for the commit b959b72 to get more accurate results

@@            Coverage Diff             @@
##             main    #3068      +/-   ##
==========================================
- Coverage   93.11%   92.67%   -0.45%     
==========================================
  Files         189      174      -15     
  Lines        6276     5555     -721     
  Branches     1319     1181     -138     
==========================================
- Hits         5844     5148     -696     
+ Misses        432      407      -25     
Impacted Files Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts 90.90% <40.00%> (-5.10%) ⬇️
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 95.49% <100.00%> (ø)
...ackages/opentelemetry-sdk-trace-base/src/config.ts 87.17% <100.00%> (+0.33%) ⬆️
...ckages/opentelemetry-sdk-trace-base/src/utility.ts 100.00% <100.00%> (ø)
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...kages/opentelemetry-sdk-trace-base/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 53.33% <0.00%> (-46.67%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 95.45% <0.00%> (-4.55%) ⬇️
...telemetry-sdk-trace-web/src/StackContextManager.ts
...ckages/opentelemetry-sdk-trace-web/karma.worker.js
... and 13 more

…ser tests

Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@svetlanabrennan svetlanabrennan marked this pull request as ready for review June 29, 2022 17:27
@svetlanabrennan svetlanabrennan requested a review from a team June 29, 2022 17:27
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for working on this! 🙂
reconfigureLimits() is a bit hard to read, but maybe some comments could help 🤔

@legendecas
Copy link
Member

I don't think this is a "chore" patch. IMO, it's more like a "feat" or "fix", from various perspectives.

@dyladan dyladan added spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization labels Jul 20, 2022
svetlanabrennan and others added 4 commits July 22, 2022 15:26
…rovider.test.ts

Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
…rovider.test.ts

Co-authored-by: Marc Pichler <marcpi@edu.aau.at>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
…an/opentelemetry-js into fix-attribute-length-issue
@svetlanabrennan svetlanabrennan changed the title chore(sdk-trace-base): fix spanLimits attribute length/count to consider env values fix(sdk-trace-base): fix spanLimits attribute length/count to consider env values Jul 22, 2022
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
…an/opentelemetry-js into fix-attribute-length-issue
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thank you for your contribution!

packages/opentelemetry-sdk-trace-base/src/utility.ts Outdated Show resolved Hide resolved
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
@dyladan dyladan merged commit 3db1056 into open-telemetry:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT env value ignored with sdk-trace-base package v1.1.0
5 participants