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

Temporarily Default to XRay Remote Sampler is Sampler is not Specified #64

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Feb 15, 2024

Issue #, if available:
Use X-Ray default sampler as default sampler if user has not specified one
Requires #55 to be merged first before this PR can work.

Description of changes:
Use X-Ray default sampler as default sampler if user has not specified one

Testing:
Assume #55 is merged

  1. Enabled span_metrics_processor and added debug statement to print out Sampled status of a span in on_start()
# aws_span_metrics_processor.py
if span.get_span_context().trace_flags.sampled:
            print("sampled")
  1. Replaced resource detectors and manually set resource with service.name=test-service-name and build ADOT SDK
  2. Setup OTel collector with XRay proxy for sampling and AWS credentials
  3. In AWS account, create sampling rule to match service_name=test-service-name
  4. Setup sample app in sample-applications/simple-client-server/server_automatic_s3client.py
  5. Update sample-applications/simple-client-server/client.py to call server/sample-app a variable number of times to verify sampling rule is applied
  6. Repeat 6 after changing sampling rule rate/reservoir

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jj22ee jj22ee requested review from AsakerMohd, srprash and a team February 15, 2024 18:20
@jj22ee
Copy link
Contributor Author

jj22ee commented Feb 15, 2024

Do not merge until #55 is merged.

Copy link
Contributor

@thpierce thpierce left a comment

Choose a reason for hiding this comment

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

Will rely on Prashant for the full review, just one comment from my quick skim.

Copy link
Contributor

@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 on the UTs. Otherwise LGTM!

@jj22ee jj22ee changed the title Default to XRay Remote Sampler is Sampler is not Specified Temporarily Default to XRay Remote Sampler is Sampler is not Specified Feb 16, 2024
@jj22ee jj22ee merged commit 5ce4162 into main Feb 16, 2024
6 checks passed
@jj22ee jj22ee deleted the default-xray-sampler branch February 16, 2024 21:50
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