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

[Sampler.AWS] AWS X-Ray remote sampler part 1 #1091

Merged
merged 27 commits into from
Mar 28, 2023

Conversation

srprash
Copy link
Contributor

@srprash srprash commented Mar 17, 2023

New feature: AWS X-Ray remote sampler.

I am going to create a series of PRs for adding the X-Ray remote sampling support for OTel via the newOpenTelemetry.Sampler.AWS package.
For more details on the X-Ray remote sampling, please refer to this doc: https://docs.aws.amazon.com/xray/latest/devguide/xray-concepts.html#xray-concepts-sampling

Changes

This PR is the first one in the series and introduces the following:

  • A basic builder and concrete class for the AWSXRayRemoteSampler. The builder provides the configuration for the polling interval and the Otel collector xray proxy endpoint.
  • The AWSXRayRemoteSampler overrides the ShouldSample method but currently throws an NotImplementedException exception when an activity/span is started.
  • The AWSXRayRemoteSampler upon initialization spins up a scheduled thread to fetch sampling rules from the X-Ray service. The sampling rules fetched are not stored anywhere in this current PR, but further implementation will build up on this logic.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes

@utpilla utpilla added the comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS label Mar 17, 2023
@srprash srprash marked this pull request as ready for review March 20, 2023 17:39
@srprash srprash requested a review from a team March 20, 2023 17:39
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #1091 (e137b5e) into main (8194f05) will increase coverage by 0.12%.
The diff coverage is 78.94%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1091      +/-   ##
==========================================
+ Coverage   71.12%   71.24%   +0.12%     
==========================================
  Files         222      229       +7     
  Lines        8360     8493     +133     
==========================================
+ Hits         5946     6051     +105     
- Misses       2414     2442      +28     
Impacted Files Coverage Δ
...OpenTelemetry.Sampler.AWS/AWSSamplerEventSource.cs 23.07% <23.07%> (ø)
.../OpenTelemetry.Sampler.AWS/AWSXRayRemoteSampler.cs 61.90% <61.90%> (ø)
.../OpenTelemetry.Sampler.AWS/AWSXRaySamplerClient.cs 88.09% <88.09%> (ø)
src/OpenTelemetry.Sampler.AWS/SamplingRule.cs 89.74% <89.74%> (ø)
...lemetry.Sampler.AWS/AWSXRayRemoteSamplerBuilder.cs 92.30% <92.30%> (ø)
...nTelemetry.Sampler.AWS/GetSamplingRulesResponse.cs 100.00% <100.00%> (ø)
...rc/OpenTelemetry.Sampler.AWS/SamplingRuleRecord.cs 100.00% <100.00%> (ø)

@Kielek Kielek changed the title [Extensions.AWSXRay] AWS X-Ray remote sampler part 1 [Samplers.AWS] AWS X-Ray remote sampler part 1 Mar 24, 2023
@Kielek Kielek added comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS and removed comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS labels Mar 24, 2023
@Kielek
Copy link
Contributor

Kielek commented Mar 24, 2023

One last thing before approval for the scaffolding, please add projects to https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/595a9a34e3e0f10f25df64153a83889ad386a3fc/.github/component_owners.yml (in alphabetical order).

@srprash srprash changed the title [Samplers.AWS] AWS X-Ray remote sampler part 1 [Sampler.AWS] AWS X-Ray remote sampler part 1 Mar 24, 2023
src/OpenTelemetry.Sampler.AWS/CHANGELOG.md Outdated Show resolved Hide resolved
src/OpenTelemetry.Sampler.AWS/README.md Outdated Show resolved Hide resolved
src/OpenTelemetry.Sampler.AWS/README.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/comp_sampler_aws.md Outdated Show resolved Hide resolved
src/OpenTelemetry.Sampler.AWS/AWSSamplerEventSource.cs Outdated Show resolved Hide resolved
srprash and others added 6 commits March 24, 2023 13:45
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
@srprash
Copy link
Contributor Author

srprash commented Mar 27, 2023

@Kielek There's some failure in the sanitycheck/misspell workflow which I believe is unrelated to these changes. Can you take a look into it? Thanks!

@Kielek
Copy link
Contributor

Kielek commented Mar 27, 2023

@srprash rerun passed. Probably network related issue

@srprash srprash requested a review from Kielek March 27, 2023 17:21
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM , as a scaffolding for further work

@srprash
Copy link
Contributor Author

srprash commented Mar 28, 2023

@Kielek So is it good to be merged? I will build upon this in future PRs.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

@utpilla , could you please check?

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

Approved with some suggestions on fixing content of README and using $(RepoRoot).

srprash and others added 3 commits March 28, 2023 14:47
Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com>
….Tests.csproj

Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants