Skip to content

Conversation

@mcculls
Copy link
Contributor

@mcculls mcculls commented May 5, 2024

What Does This Do

Merge of #6987 and #6989 for testing purposes

Motivation

Forthcoming changes will allow rules to be set via remote configuration.
Once rules can be either locally or remotely, we want the decision maker to reflect the source of the rule.

Additional Notes

Change makes extensive changes to the sampling rule tests - previously these tests weren't checking the decision maker tag

Jira ticket: AIT-9975

@mcculls mcculls added the comp: core Tracer core label May 5, 2024
@mcculls mcculls force-pushed the mcculls/handle-remote-sampling-rules_MERGE branch from aedbe1b to cb12ad7 Compare May 5, 2024 20:44
rule.getTags(),
new DeterministicSampler.TraceSampler(rule.getSampleRate()));
new DeterministicSampler.TraceSampler(rule.getSampleRate()),
samplingMechanism(rule.getProvenance()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good

final SamplingRule samplingRule =
new ServiceSamplingRule(
final RateSamplingRule samplingRule =
new RateSamplingRule.ServiceSamplingRule(
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't quite decide what I wanted to do with ServiceSamplingRule and OperationSamplingRule. I thought about explicitly specifying the mechanism just to make the logic more clear, but I'll leave it to you to decide.

@bm1549 bm1549 marked this pull request as ready for review May 6, 2024 15:24
@bm1549 bm1549 requested a review from a team as a code owner May 6, 2024 15:24
@bm1549 bm1549 requested review from dougqh and nayeem-kamal May 6, 2024 15:24
@mcculls
Copy link
Contributor Author

mcculls commented May 6, 2024

Testing complete, PRs will be merged separately

@mcculls mcculls closed this May 6, 2024
@mcculls mcculls deleted the mcculls/handle-remote-sampling-rules_MERGE branch May 6, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants