-
Notifications
You must be signed in to change notification settings - Fork 805
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
fix(sdk-trace-base): fix spanLimits attribute length/count to consider env values #3068
Conversation
…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 Report
@@ 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
|
…ser tests Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
Signed-off-by: Svetlana Brennan <50715937+svetlanabrennan@users.noreply.github.com>
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.
Looks good, thanks for working on this! 🙂
reconfigureLimits()
is a bit hard to read, but maybe some comments could help 🤔
packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts
Outdated
Show resolved
Hide resolved
I don't think this is a "chore" patch. IMO, it's more like a "feat" or "fix", from various perspectives. |
…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
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>
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.
Overall LGTM. Thank you for your contribution!
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>
Which problem is this PR solving?
OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT
andOTEL_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 updatingspanLimits.attributeValueLengthLimit
andspanLimits.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 limitOTEL_ATTRIBUTE_COUNT_LIMIT
.NOW
Now it checks if the user did not set the
spanLimits.attributeValueLengthLimit
programmatically AND ifOTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT
env value is still set to the default value (ex. Infinity) BUT if the user set thegeneralLimits.attributeValueLengthLimit
programmatically OR user set theOTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT
via env, then reconfigure thespanLimits.attributeValueLengthLimit
value to either use thegeneralLimits.attributeValueLengthLimit
set programmatically or use theOTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT
env value.Note: I had to wrap the old
DEFAULT_CONFIG
object into a function because those envs value (parsed inDEFAULT_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.
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
Checklist: