Trace SDK does not raise when Span attribute and link limits are exceeded#1856
Conversation
use deque extend for boundlist initialization better variable names cleaner tests more focused tests
|
|
|
If we are choosing to drop the extra attributes, we must implement the following line as well:
Might need to be coordinated with this pr. |
Agreed! Looks to me like this change set would be cleanest done as a followup to 1839. I might just sit on this until the changes to span limits are merged, then rebase and redo these changes with adjustments. That sound reasonable to you? |
|
@lzchen looks like the scope of #1839 has changed and does not appear to satisfy the intent of the spec you mentioned. Thoughts on whether or not we should park this issue until we can provide a reasonable way to configure span limits on a TraceProvider? As I understand it, we're already in an awkward place with compliance to this given this test is passing test_span_environment_limits. |
|
@c-kruse I removed that functionality in order to speed up the PR review process. I intent to start separate discussion tomorrow around what the TracerProvider API should look like and submit a new PR after that. |
Description
Relevant Spec here.
We should not be raising
ValueErrorwhen starting a span if the number of links or attributes exceeds the configured maxima. Surplus attributes and links can be discarded instead.opentelemetry.sdk.util.BoundedList.from_seq(...)behavior changes to not raise ValueError. Instead usesBoundedList.extendto fill the deque to capacity and preserve a dropped count for excess items.opentelemetry.sdk.util.BoundedDict.from_map(...)behavior changes to not raise ValueError. Instead it iterates on the input mapping and inserts keys one by one relying onBoundedDict.__setitem__to discard excess items.Fixes #1844
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
toxtest suiteDoes This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/oropentelemetry-sdk/The method interfaces of
test/utilhave changedScripts in
scripts/that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.tomlisort.cfg.flake8When a new
.github/CODEOWNERis addedMajor changes to project information, such as in:
README.mdCONTRIBUTING.mdYes. - Link to PR:
No.
I'm not positive - could use some help on this front
Checklist: