Skip to content

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Apr 22, 2025

Description

This is the 2nd PR to address #1434 in order for OTel Ruby to support X-Ray Remote Sampling. One more PR to fully implement this feature will follow this one.

Changes:

  • Add Sampling RuleCache
    • Caches a list of SamplingRuleAppliers, ordered by rule priority then rule name. Each Rule Applier corresponds to the Sampling Rule from GetSamplingRules. Each call to GetSamplingRules will only update the Rules that have changed properties, to preserve the state of unchanged rules. This means when Reservoir and Statistics are implemented (later) in the Rules, they will persist for unchanged rules.
    • The RuleCache will determine which Rule a set of {ResourceAttributes,SpanAttributes} matches with that has highest priority.
  • Update SamplingRuleApplier to perform Fixed Rate Sampling, and to include a method to apply matching logic against a set of {ResourceAttributes,SpanAttributes} by using the wild card and attribute matching from Utils
  • Initial class for FallbackSampler
  • Update X-Ray Remote Sampler to depend on the SamplingRuleApplier from RuleCache and the FallBack Sampler to perform should_sample?

Testing

  • Unit Tests
  • The current implementation was tested against X-Ray's Centralized Sampling Integration tests. The tests for applying Fixed Rate sampling from X-Ray Sampling Rules worked, while the Reservoir sampling from X-Ray Sampling Rules failed (expected since the implementation is incomplete)

@jj22ee
Copy link
Contributor Author

jj22ee commented Apr 22, 2025

@lukeina2z Need your help reviewing. Currently everything is implemented except for Updating Sampling Targets and the Rate Limiting (Reservoir) Sampler.

@jj22ee
Copy link
Contributor Author

jj22ee commented Apr 28, 2025

PR is ready for merge from our end.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Overall, this looks great! I have a few small questions/comments.

Thanks for your patience!

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and for your patience @jj22ee. Looking forward to the next PR!

@kaylareopelle kaylareopelle enabled auto-merge (squash) May 14, 2025 18:58
@kaylareopelle kaylareopelle merged commit a0b2477 into open-telemetry:main May 14, 2025
62 checks passed
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.

3 participants