Skip to content

feat(opentelemetry-sampler-aws-xray): Add Rate Limiter and Sampling Targets Poller Logic #2924

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Jul 3, 2025

Which problem is this PR solving?

Short description of the changes

This PR is a followup to #2824

  • Added logic to fetch sampling targets for each SamplingRuleApplier.
    • The sampling targets are periodically fetched every 10 seconds by making the GetSamplingTargets API call to X-Ray.
    • The targets determine the reservoir quota and the rate at which a SamplingRuleApplier will sample the requests.
    • Each rule applier keeps and updates a sampling statistics document which is required in GetSamplingTargets call to determine the next target
  • Added a RateLimitingSampler (applied before the fixed rate sampler) to be used in each rule applier.
    • Together these samplers determine how many requests to sample every second and what percentage of additional requests to sample in that second.
    • The FallbackSampler is updated to be a combination of above samplers to sample 1 req/sec and 5% of additional requests in that second.

This PR's implementation was tested against X-Ray's Centralized Sampling Integration tests.

@jj22ee jj22ee requested a review from a team as a code owner July 3, 2025 18:31
@github-actions github-actions bot requested a review from yiyuan-he July 3, 2025 18:31
Copy link

codecov bot commented Jul 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.06%. Comparing base (e9987a7) to head (a2c0165).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2924   +/-   ##
=======================================
  Coverage   89.06%   89.06%           
=======================================
  Files         188      188           
  Lines        9219     9219           
  Branches     1900     1900           
=======================================
  Hits         8211     8211           
  Misses       1008     1008           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


import { SamplingRuleApplier } from './sampling-rule-applier';
import { PACKAGE_NAME } from './version';
Copy link
Contributor

Choose a reason for hiding this comment

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

How this file is generated? I tried 'npm run setup:dev' and 'npm run compile', none of them generate this file and that is causing build failure.

Copy link
Contributor Author

@jj22ee jj22ee Jul 21, 2025

Choose a reason for hiding this comment

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

It is generated by npm run version:update. However, while this package is in the incubator folder, this command isn't automatically run for this package until it is moved out of this directory.

}

public toString(): string {
return `RateLimitingSampler{rate limiting sampling with sampling config of ${this.quota} req/sec and 0% of additional requests}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need 'and 0% of additional requests' in the context of rate limiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I kept this text to be aligned with the Python implementation.

@dyladan
Copy link
Member

dyladan commented Jul 16, 2025

@jj22ee I see lukeina2z is also an AWS employee. Should he be considered a component owner or SME for this PR? I think this will be mergeable if so when the comments are addressed.

@jj22ee
Copy link
Contributor Author

jj22ee commented Jul 21, 2025

@dyladan Yes, Luke is from AWS, and he can be considered as an SME for this PR. Will be addressing his comments soon.

Copy link
Contributor

@yiyuan-he yiyuan-he left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants