xray: fix the default sampling rate for AWS X-Ray tracer by suniltheta · Pull Request #15958 · envoyproxy/envoy

suniltheta · 2021-04-13T19:40:40Z

Signed-off-by: Sunil Narasimhamurthy sunnrs@amazon.com

Commit Message: fix the default sampling rate for AWS X-Ray tracer extension to 5%
Additional Description:
Risk Level: Low
Testing: unit test the LocalizedSamplingStrategy to check if the default sampling rate is 0.05 if no other localized rule is applied via configuration
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

and test for the same if no localized sampling rule is applied. Update the release ntoes with the bug fix information Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tonya11en · 2021-04-13T20:00:27Z

source/extensions/tracers/xray/localized_sampling.h

+  /**
+   * @return the default manifest. Mainly for unit testing purposes.
+   */
+  LocalizedSamplingManifest& defaultManifest() { return default_manifest_; }


Since this is just used for validation in tests, can you make this a const LocalizedSamplingManifest& defaultManifest() const.

Thanks for the tip. I made this change.

tonya11en · 2021-04-13T22:43:23Z

docs/root/version_history/current.rst

 * tls: fix issue where OCSP was inadvertently removed from SSL response in multi-context scenarios.
 * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.
 * upstream: retry budgets will now set default values for xDS configurations.
+* xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5%.


Is there a reason this is under "bug fixes" and not "minor behavioral changes"?

As per the design proposal #5644 and documented behavior, the default sampling rate for AWS X-Ray tracer is expected to be 5%. It was a typo/bug in the code that the DefaultRate value was 0.5 (50%) and not 0.05 (5%). Which will be addressed by this PR.

Consider appending something like "as opposed to 50%" so it's clear that was the bug.

tonya11en · 2021-04-13T23:02:36Z

/wait-any

unit testing to be a const and add a small code comment Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta · 2021-04-16T05:02:18Z

Hi @tonya11en due to the release v1.18 that went today there was a merge conflict in docs/root/version_history/current.rst file which I just resolved. Can you please review the changes again and approve? Thank you for your time.

tonya11en

Thanks for updating! This mostly LGTM, I just had questions/suggestions.

/wait-any

tonya11en · 2021-04-16T07:16:42Z

docs/root/version_history/current.rst

 * tls: fix issue where OCSP was inadvertently removed from SSL response in multi-context scenarios.
 * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.
 * upstream: retry budgets will now set default values for xDS configurations.
+* xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5%.


Consider appending something like "as opposed to 50%" so it's clear that was the bug.

tonya11en · 2021-04-16T07:19:55Z

source/extensions/tracers/xray/localized_sampling.cc

 namespace XRay {

-constexpr double DefaultRate = 0.5;
+// DefaultRate=0.05 corresponds to 5% sampling rate when no custom rules are applied


nit: End with a period.

You also can just start with // Corresponds to....

It's not clear why only this variable gets an explanation, but the others do not. The JSON keys are obvious, but DefaultFixedTarget and SamplingFileVersion may as well get an explanation too.

Added the comments (explanation) for the other two variables as well. 👍

And for the previous review comment > Consider appending something like ... I made the change in the latest commit. For some reason I was not able to directly reply to that review comment.

tonya11en · 2021-04-16T07:22:05Z

source/extensions/tracers/xray/localized_sampling.h

+  /**
+   * @return default sampling rule's sampling rate. Mainly for unit testing purposes.
+   */
+  double defaultRuleRate() const { return default_rule_.rate(); }
+


Is this necessary? Why can't you just call defaultRule().rate()?

From earlier suggestion in this PR, the function LocalizedSamplingStrategy::defaultManifest() function was made a const.
const LocalizedSamplingManifest& defaultManifest() const.

So the return type is a const reference of LocalizedSamplingManifest object. To make a call strategy.defaultManifest().defaultRule().rate() in the UnitTest code, the function LocalizedSamplingManifest::defaultRule() was not expected to be called by a const reference of LocalizedSamplingManifest object.

Instead of making defaultRule function as const like this ->LocalizedSamplingManifest::defaultRule const() which in turn needed multiple changes where defaultRule function is called within the source code, I opted to creating a new function double defaultRuleRate() const {} which seemed like a less code change.

Can you not just make a copy of the default rule in the unit test from the cost ref?

Is this necessary? Why can't you just call defaultRule().rate()?

It is not necessary. In the new commit, got rid of this function (defaultRuleRate) and instead making a copy of the default_manifest_ from the const ref

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

…d make a copy of const object for the unit testing purpose Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tonya11en

Thanks! It's left in a better state than it was found, too!

LGTM assuming CI passes and all that.

suniltheta · 2021-04-16T22:53:02Z

Once again thanks for the review and suggestions, especially the hints for making it more readable.

suniltheta · 2021-04-20T16:20:38Z

Hi @tonya11en, I resolved a new merge conflict in docs/root/version_history/current.rst file. Can you please approve once again?
Any tip to get attention from maintainers to get this PR merged, in a nice way?

tonya11en

Looks like there's another conflict :(. There's a lot of churn right now in the release notes because we're setting up the new versions. Thanks for being patient.

I'll ping a maintainer once that's all fixed.

tonya11en · 2021-04-20T20:41:17Z

docs/root/version_history/current.rst

+* xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5% as opposed to 50%.
 * zipkin: fix timestamp serializaiton in annotations. A prior bug fix exposed an issue with timestamps being serialized as strings.

+


rm extra line

Removed this extra line and resolved the merge conflict.

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tonya11en

@asraa since Matt's out of town, can another maintainer take the last pass?

asraa · 2021-04-21T21:33:22Z

Sure! Changes LGTM, would like to get an xray extension codeowner to check @abaptiste
But if not ping me and I will approve.

abaptiste · 2021-04-22T15:01:03Z

@asraa @tonya11en LGTM

suniltheta · 2021-04-22T16:35:47Z

thanks @abaptiste @asraa @tonya11en for your time.

…15958) * Fix the default sampling rate for AWS X-Ray tracer extension to 5% and test for the same if no localized sampling rule is applied. Update the release notes with the bug fix information Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

change the default sampling rate for AWS X-Ray tracer extension to 5%

e720a02

and test for the same if no localized sampling rule is applied. Update the release ntoes with the bug fix information Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta requested a review from mattklein123 as a code owner April 13, 2021 19:40

mattklein123 assigned tonya11en Apr 13, 2021

suniltheta changed the title ~~bug: fix the default sampling rate for AWS X-Ray tracer~~ xray: fix the default sampling rate for AWS X-Ray tracer Apr 13, 2021

tonya11en reviewed Apr 13, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Apr 13, 2021

address a PR review comment to make a function used only for

5e7269b

unit testing to be a const and add a small code comment Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot removed the waiting:any label Apr 14, 2021

tonya11en previously approved these changes Apr 14, 2021

View reviewed changes

Merge branch 'main' into awsxray_default_sampling_rate

791a715

suniltheta dismissed tonya11en’s stale review via 791a715 April 16, 2021 04:55

suniltheta requested a review from tonya11en April 16, 2021 04:58

tonya11en reviewed Apr 16, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Apr 16, 2021

Address the PR review comment, to make these changes more understandable

7beb541

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot removed the waiting:any label Apr 16, 2021

Sunil Narasimhamurthy added 2 commits April 16, 2021 22:43

Get rid of defaultRuleRate() function which is unnecessary and instea…

6a35f45

…d make a copy of const object for the unit testing purpose Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

remove unnecessary line in a header file

7e108e3

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tonya11en previously approved these changes Apr 16, 2021

View reviewed changes

Merge branch 'main' into awsxray_default_sampling_rate

e787381

suniltheta dismissed tonya11en’s stale review via e787381 April 20, 2021 16:13

suniltheta requested a review from tonya11en April 20, 2021 16:14

tonya11en requested changes Apr 20, 2021

View reviewed changes

Merge branch 'main' into awsxray_default_sampling_rate

b0c2c61

suniltheta requested a review from tonya11en April 20, 2021 20:51

dummy commit to restart CI

d68055d

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

tonya11en approved these changes Apr 21, 2021

View reviewed changes

asraa assigned asraa and abaptiste Apr 21, 2021

asraa approved these changes Apr 22, 2021

View reviewed changes

asraa merged commit 118f66e into envoyproxy:main Apr 22, 2021

suniltheta mentioned this pull request Apr 23, 2021

Bug: high default sampling rate for AWS X-Ray tracer in envoy proxy aws/aws-app-mesh-roadmap#338

Closed

suniltheta deleted the awsxray_default_sampling_rate branch February 13, 2022 02:53

		* xray: fix the default sampling 'rate' for AWS X-Ray tracer extension to be 5% as opposed to 50%.
		* zipkin: fix timestamp serializaiton in annotations. A prior bug fix exposed an issue with timestamps being serialized as strings.

Conversation

suniltheta commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Apr 13, 2021

Uh oh!

suniltheta commented Apr 16, 2021

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suniltheta Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

suniltheta commented Apr 16, 2021

Uh oh!

suniltheta commented Apr 20, 2021

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

asraa commented Apr 21, 2021

Uh oh!

abaptiste commented Apr 22, 2021

Uh oh!

suniltheta commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

suniltheta Apr 16, 2021 •

edited

Loading

suniltheta commented Apr 22, 2021 •

edited

Loading