[processor/tailsampling] Give composite sub-policies omitted from rat… - #49857
Open
Atishyy27 wants to merge 2 commits into
Open
[processor/tailsampling] Give composite sub-policies omitted from rat…#49857Atishyy27 wants to merge 2 commits into
Atishyy27 wants to merge 2 commits into
Conversation
…e_allocation their default share getRateAllocationMap iterated only over RateAllocation entries, so any sub-policy not listed there was never assigned a MaxSpansPerSecond and defaulted to 0, permanently blocking it from sampling. Iterate over every configured sub-policy instead, falling back to the equal-share default (MaxTotalSpansPerSecond / number of sub-policies) already used for entries with Percent == 0. Fixes open-telemetry#49828.
Contributor
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
|
Pull request dashboard statusStatus last refreshed: 2026-07-27 21:18:55 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected. |
Member
|
/workflow-approve |
Member
|
Please fix the linter issue. |
golangci-lint (gocritic rangeValCopy) flagged the range-over-SubPolicyCfg loop copying a 456-byte struct each iteration. Iterate by index and take a pointer instead. No behavior change.
Author
|
Switched to indexed iteration so the sub-policy loop no longer copies the struct, addressing the linter warning. This was fixed in commit 2b25da9. @singhvibhanshu, could you please review and re-run/approve the workflow when you have a chance? The latest run is waiting for maintainer approval. |
Member
|
/workflow-approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
getRateAllocationMappreviously iterated only over entries inrate_allocation. As a result, any configured composite sub-policy that was omitted fromrate_allocationwas never assigned aMaxSpansPerSecondvalue and implicitly defaulted to0, preventing it from sampling any spans.The documented behavior is that sub-policies without an explicit allocation receive the default equal share (
max_total_spans_per_second / number of sub-policies). This is also the behavior already applied to entries withpercent: 0.This change iterates over every configured sub-policy, applying the configured percentage when present and falling back to the default equal-share allocation otherwise.
Link to tracking issue
Fixes #49828
Testing
Added a
TestCompositeHelpertest case covering the scenario where a configured sub-policy is omitted fromrate_allocation. The test configures two sub-policies with a total rate of1000spans/second, assigns one policy an explicit allocation of25%, and verifies that the omitted policy receives the default allocation of500(1000 / 2) rather than0.The test fails with the previous implementation and passes with this change.
Documentation
No documentation changes were required. This change aligns the implementation with the existing documented behavior for composite rate allocation.
Authorship