Skip to content

AWS X-Ray Remote Sampler Part 1 - Initial Classes and Rules Poller Implementation #3366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Aug 25, 2025
Merged

AWS X-Ray Remote Sampler Part 1 - Initial Classes and Rules Poller Implementation #3366

merged 23 commits into from
Aug 25, 2025
+1,632 −6

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Mar 15, 2025 •
edited
Loading

Description

This is an initial PR to address #3305 in order for OTel Python to support X-Ray Remote Sampling. A series of PRs to fully implement this feature will follow.

Changes:

  • Add xray sampler under opentelemetry-sdk-extension-aws
  • Code changes:
    • Add Sampling Client class to communicate with X-Ray Service for Sampling Rules/Targets
    • Add X-Ray Sampling Rules/Targets/Statistics class to help represent the Sampling related data from X-Ray
    • Added the AwsXRayRemoteSampler skeleton class
      • Ensure AwsXRayRemoteSampler is a ParentBased Sampler, with internal logic inside _AWSXRayRemoteSampler
      • Run poller upon instantiation to regularly obtain X-Ray Sampling Rules from X-Ray

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

This PR is not a full implementation of the sampler, so it cannot be tested for correct functionality.
Right now, it currently always return a Parent Based decision or a DROP decision as a placeholder.

  • Unit Tests

A couple more PRs will follow-up to complete the implementation, which should look like this Sampler code from the AWS ADOT repo. At that point in the future, a full E2E integration test will be run.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@jj22ee jj22ee requested a review from a team as a code owner March 15, 2025 12:31
@jj22ee jj22ee removed their assignment Mar 16, 2025
@xrmx
Copy link
Contributor

xrmx commented Mar 17, 2025

Question: would it make sense to add this to https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/sdk-extension/opentelemetry-sdk-extension-aws instead of creating a new package?

@jj22ee
Copy link
Contributor Author

jj22ee commented Mar 18, 2025

Yeah that makes sense, opentelemetry-sdk-extension-aws seems to hold all aws components... except the AWS XRay Propagator that I based my work off of...

I synced with package owner @srprash, there is no concern with moving this Sampler to the SDK Extensions.

@jj22ee jj22ee removed their assignment Mar 19, 2025
srprash
srprash reviewed Apr 16, 2025
View reviewed changes
Copy link

@srprash srprash left a comment

Choose a reason for hiding this comment

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

Minor comments. Overall LGTM.

@jj22ee
Copy link
Contributor Author

jj22ee commented Apr 29, 2025

@xrmx looking for approval from maintainers, the PR has component owner approval.

xrmx
xrmx reviewed May 26, 2025
View reviewed changes
@jj22ee
Copy link
Contributor Author

jj22ee commented Jun 4, 2025

@xrmx Since this is a partial implementation, what is a good way to hide this new AwsXRayRemoteSampler class from users? Can I give it a dummy name or just prefix it with an underscore until the implementation is complete?

@jj22ee
Copy link
Contributor Author

jj22ee commented Jun 17, 2025

@xrmx @srprash Since the sampler is currently partially implemented, I've renamed the 2 X-Ray Sampler classes to be marked as internal for now via the following changes:

  • _AwsXRayRemoteSampler -> _InternalAwsXRayRemoteSampler
  • AwsXRayRemoteSampler -> _AwsXRayRemoteSampler

When the implementation is complete, I will rename _AwsXRayRemoteSampler back to AwsXRayRemoteSampler.
Can this PR be merged?

@xrmx xrmx moved this to Ready for review in @xrmx's Python PR digest Jun 24, 2025
xrmx
xrmx reviewed Jun 26, 2025
View reviewed changes
xrmx
xrmx reviewed Jun 26, 2025
View reviewed changes
NathanielRN
NathanielRN reviewed Jun 26, 2025
View reviewed changes
@jj22ee
Copy link
Contributor Author

jj22ee commented Aug 4, 2025

@xrmx Looking for merge! Planning the submit one more PR after this to complete the implementation.

@xrmx xrmx merged commit 032d6c6 into open-telemetry:main Aug 25, 2025
1195 of 1198 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in @xrmx's Python PR digest Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xrmx xrmx xrmx approved these changes

+2 more reviewers

@NathanielRN NathanielRN NathanielRN left review comments

@srprash srprash srprash approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@NathanielRN NathanielRN

@srprash srprash

@jj22ee jj22ee

Labels
None yet
Projects
@xrmx's Python PR digest
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jj22ee @xrmx @NathanielRN @srprash