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

Update span limits to comply with specification #2637

Merged
merged 31 commits into from Mar 3, 2022

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 23, 2022

Fix #2635
Fix #2636

  • Support these additional environment variables:
    • OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT
    • OTEL_EVENT_ATTRIBUTE_COUNT_LIMIT
    • OTEL_LINK_ATTRIBUTE_COUNT_LIMIT
  • Deprecated WithSpanLimits in favor of the new WithRawSpanLimits. WithRawSpanLimits supports 0 and unlimited limits. WithSpanLimits is updated to preserve existing behavior.
  • Add span attribute value length limit.

Testing

  • Benchmarks added for all limits.
  • Unified and consolidated span limit setting tests.
    • Added tests for missing test cases
  • Added "integration" tests to ensure exported spans with limits are as expected.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #2637 (7b57418) into main (24414b2) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2637   +/-   ##
=====================================
  Coverage   75.6%   75.7%           
=====================================
  Files        172     172           
  Lines      11551   11619   +68     
=====================================
+ Hits        8736    8798   +62     
- Misses      2605    2611    +6     
  Partials     210     210           
Impacted Files Coverage Δ
sdk/internal/env/env.go 100.0% <100.0%> (ø)
sdk/trace/evictedqueue.go 100.0% <100.0%> (ø)
sdk/trace/provider.go 84.4% <100.0%> (+1.8%) ⬆️
sdk/trace/span.go 87.8% <100.0%> (+1.3%) ⬆️
sdk/trace/span_limits.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-1.9%) ⬇️
exporters/jaeger/jaeger.go 90.3% <0.0%> (-0.9%) ⬇️

@MrAlias MrAlias changed the title PoC for span limit refactor Update span limits to comply with specification Feb 25, 2022
@MrAlias MrAlias added this to the Release v1.5.0 milestone Feb 25, 2022
@MrAlias MrAlias marked this pull request as ready for review February 25, 2022 00:44
Copy link
Contributor

@MadVikingGod MadVikingGod 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.

The only comment that stops me from approving is: mutating the attribute. the others are mostly color choices.

sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/span.go Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
sdk/trace/span_limits.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias MrAlias merged commit 0d0a732 into open-telemetry:main Mar 3, 2022
@MrAlias MrAlias deleted the unlimited-limit-vals branch March 3, 2022 15:56
hanyuancheung pushed a commit to hanyuancheung/opentelemetry-go that referenced this pull request Mar 5, 2022
* PoC for span limit refactor

* Rename config.go to span_limits.go

* Add unit tests for truncateAttr

* Add unit tests for non-string attrs

* Add span limit benchmark tests

* Fix lint

* Isolate span limit tests

* Clean span limits test

* Test limits on exported spans

* Remove duplicate test code

* Fix lint

* Add WithRawSpanLimits option

* Add test for raw and orig span limits opts

* Add changes to changelog

* Add tests for span resource disabled

* Test unlimited instead of default limit

* Update docs

* Add fix to changelog

* Fix option docs

* Do no mutate attribute

* Fix truncateAttr comment

* Remake NewSpanLimits to be newEnvSpanLimits

Update and unify documentation accordingly.

* Update truncateAttr string slice update comment

* Update CHANGELOG.md

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@MrAlias MrAlias mentioned this pull request Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "unlimited" limit values Add limit for span attribute value length
5 participants