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

Links based sampler #813

Merged
merged 9 commits into from
May 23, 2023
Merged

Links based sampler #813

merged 9 commits into from
May 23, 2023

Conversation

atshaw43
Copy link
Contributor

@atshaw43 atshaw43 commented Apr 6, 2023

Description:
Feature addition

Problem
What if we only want to sample if and only if one of the span links is sampled.

Solution
Add a LinksBasedSampler sampler that samples if and only if a span link is sampled.

Testing:
Unit Tests

@atshaw43 atshaw43 requested a review from a team April 6, 2023 18:15
@github-actions github-actions bot requested review from iNikem and trask April 6, 2023 18:15
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Couple minor comments but looks good.

Comment on lines +24 to +25
private static final SamplingResult POSITIVE_SAMPLING_RESULT = SamplingResult.recordAndSample();
private static final SamplingResult NEGATIVE_SAMPLING_RESULT = SamplingResult.drop();
Copy link
Member

@trask trask Apr 28, 2023

Choose a reason for hiding this comment

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

I don't think these constants help improve readability or performance here, so I'd suggest inlining

Copy link
Member

Choose a reason for hiding this comment

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

@atshaw43 just checking in to make sure you saw this comment, thx

@trask trask merged commit 22d0f82 into open-telemetry:main May 23, 2023
@trask trask mentioned this pull request May 23, 2023
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