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

feat(opentelemetry-sampler-aws-xray): add x-ray remote sampler #1443

Merged
merged 13 commits into from
May 11, 2023

Conversation

carolabadeer
Copy link
Contributor

@carolabadeer carolabadeer commented Mar 28, 2023

Which problem is this PR solving?

Short description of the changes

  • Created a new opentelemetry-sampler-aws-xray package to vend this new component since all AWS components in this repo have been split into their own packages.
  • Added the AWSXRayRemoteSampler skeleton class, which will have more logic added in the next PR for matching sampling rules.
    • The shouldSample and toString methods have been implemented, but currently return values that will be updated in coming PRs.
    • The AWSXRayRemoteSampler allows for specifying a polling interval and will fetch sampling rules periodically using that interval using the setInterval function.
  • The remote-sampler.types.ts file houses all the interfaces / data models that the X-Ray remote sampler uses. All interfaces will be defined in this file and additional interfaces will also be added there.

@carolabadeer carolabadeer requested a review from a team March 28, 2023 17:29
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Mostly minor comments, skeleton looks good though!

Copy link
Member

@blumamir blumamir 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 contributing this component.
Left comments on the PR, please write if anything is not clear.

Please also add yourself as the component owner in this file. This means you will be responsible to review and maintain this component in the future.

@codecov
Copy link

codecov bot commented Apr 2, 2023

Codecov Report

Merging #1443 (0dc2a36) into main (4eb405e) will decrease coverage by 0.06%.
The diff coverage is 94.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   96.13%   96.08%   -0.06%     
==========================================
  Files          14       15       +1     
  Lines         906      945      +39     
  Branches      197      203       +6     
==========================================
+ Hits          871      908      +37     
- Misses         35       37       +2     
Impacted Files Coverage Δ
...entelemetry-sampler-aws-xray/src/remote-sampler.ts 94.87% <94.87%> (ø)

@carolabadeer carolabadeer changed the title feat(opentelemetry-remote-sampler-aws-xray): add x-ray remote sampler feat(opentelemetry-sampler-aws-xray): add x-ray remote sampler Apr 7, 2023
private _endpoint: string;

constructor(samplerConfig: AWSXRaySamplerConfig) {
this._pollingInterval =
Copy link
Member

Choose a reason for hiding this comment

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

maybe add some sanity check to avoid negative or terrible small values?


// fetch sampling rules every polling interval
private getAndUpdateSamplingRules(): void {
setInterval(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work reliable in AWS lambda where the runtime simply freezes the process once the handler returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AWS Lambda does not support remote sampling, it uses a fixed sampling rate

Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention this incompatibility in the readme.
At least from the package name it's not clear to me that AWS lambda is unsupported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I added a note in the readme to indicate that AWS Lambda does not support remote sampling

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with sampling, but the skeleton looks good. 🙂
Looking forward to the follow-up PRs.

packages/opentelemetry-sampler-aws-xray/README.md Outdated Show resolved Hide resolved
@carolabadeer carolabadeer force-pushed the xray-remote-sampling branch from d4a9127 to e1ff849 Compare May 5, 2023 01:34
@carolabadeer carolabadeer force-pushed the xray-remote-sampling branch from e1ff849 to 0cc5d27 Compare May 5, 2023 01:39
@pichlermarc pichlermarc merged commit 79cd677 into open-telemetry:main May 11, 2023
@dyladan dyladan mentioned this pull request Feb 13, 2024
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.

5 participants