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.

Sign up for GitHub

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

Jump to bottom

fix(tracing): sample rate not working #10485

Merged
merged 10 commits into from
Mar 28, 2023
Merged

fix(tracing): sample rate not working #10485

merged 10 commits into from
Mar 28, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Mar 15, 2023 •
edited
Loading

Full change log

  • Fixed the original sampling algorithm implementation and added a test to examine the precision
  • Removing FLAG_SAMPLED_AND_RECORDING flags as it seems we do not distinguish sample and recording yet, and it causes the result always be "not sampling".

Checklist

Issue reference

Fix #10402
Fix KAG-910

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 15, 2023
@StarlightIbuki
Copy link
Contributor Author

StarlightIbuki commented Mar 15, 2023 •
edited
Loading

Though still in doubt about the correctness of the algorithm (another doc is here), I fixed the original sampling implementation and added a test to examine the precision.

@StarlightIbuki StarlightIbuki force-pushed the fix/tracing-sample-rate branch from 6df4d03 to cacdb05 Compare March 15, 2023 08:08
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 15, 2023
@StarlightIbuki StarlightIbuki force-pushed the fix/tracing-sample-rate branch 2 times, most recently from 4cb601c to 6ec86f4 Compare March 16, 2023 04:09
@StarlightIbuki StarlightIbuki requested a review from bungle March 17, 2023 02:34
bungle
bungle previously requested changes Mar 17, 2023
View reviewed changes
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

Just checking

@StarlightIbuki StarlightIbuki requested a review from bungle March 20, 2023 02:43
@bungle bungle dismissed their stale review March 21, 2023 07:21

fixed

bungle
bungle approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

I am not expert in tracing, but I don't see issues here.

@ghost
Copy link

ghost commented Mar 21, 2023

I have reviewed this code and it is consistent with the description provided by probability-sampler-algorithm. The logic is correct and I believe it will work well.

@StarlightIbuki StarlightIbuki force-pushed the fix/tracing-sample-rate branch 2 times, most recently from c8ac985 to 521bbb5 Compare March 23, 2023 03:41
samugi
samugi reviewed Mar 24, 2023
View reviewed changes
samugi
samugi requested changes Mar 24, 2023
View reviewed changes
Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

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

Looks great to me. An adjustment is required in a test parameter otherwise I think it'll be flaky. Changelog conflicts should be resolved too.

@StarlightIbuki StarlightIbuki force-pushed the fix/tracing-sample-rate branch from 521bbb5 to f2ec795 Compare March 27, 2023 02:55
@StarlightIbuki StarlightIbuki requested a review from samugi March 27, 2023 02:59
@samugi samugi merged commit 2b6ed5c into master Mar 28, 2023
@samugi samugi deleted the fix/tracing-sample-rate branch March 28, 2023 04:46
@hbagdi
Copy link
Member

hbagdi commented Mar 28, 2023

Please cherry-pick this to EE.

@hbagdi
Copy link
Member

hbagdi commented Mar 29, 2023

ping @StarlightIbuki

@StarlightIbuki
Copy link
Contributor Author

ping @StarlightIbuki

PR created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bungle bungle bungle approved these changes

@samugi samugi samugi approved these changes

Assignees

@StarlightIbuki StarlightIbuki

Labels
changelog core/pdk core/tracing size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenTelemetry plugin does not send out any span while 'tracing_sampling_rate' is lower than 1.0
4 participants
@StarlightIbuki @hbagdi @bungle @samugi