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

fix random sample fraction percent #102

Merged
merged 3 commits into from
Sep 11, 2019

Conversation

bianpengyuan
Copy link

@douglas-reid
Copy link

@bianpengyuan i'm assuming this change has been manually validated.

@@ -266,8 +266,10 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
envoy::type::FractionalPercent random_sampling;
// TODO: Random sampling historically was an integer and default to out of 10,000. We should
// deprecate that and move to a straight fractional percent config.
uint64_t random_sampling_numerator{PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT(

Choose a reason for hiding this comment

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

just for my sanity...
if tracing_config().random_sampling().value() == 100 then This thing will return MAX_VALUE == 10000
which would set the numerator correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, see the test that I added.

Copy link

@mandarjog mandarjog left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Can you submit this change to upstream and then cherry-pick? I don't think we should do this kind of behavior change in istio fork of envoy.

Copy link

@lizan lizan left a comment

Choose a reason for hiding this comment

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

I meant request changes, not approval

@mandarjog
Copy link

mandarjog commented Sep 11, 2019

@lizan this is holding up the release. Looks like upstream changed behaviour and there were no tests. Do you feel that this is wrong fix, or would you want to see this in upstream first?

@lizan
Copy link

lizan commented Sep 11, 2019

Get this reviewed in upstream together, and also, add a test please.

@douglas-reid
Copy link

@lizan is there a way to fast-track the Envoy PR review somehow, if we add the tests?

@lizan
Copy link

lizan commented Sep 11, 2019

@douglas-reid I'm happy to review and I think this makes sense, so it shouldn't take long. I'm fine to merge this earlier than upstream PR, as long as we have upstream PR tracked and merge back if any change requested in upstream.

@mandarjog
Copy link

We will do that, thanks @lizan

so @bianpengyuan

  1. Create a parallel PR to upstream with tests
  2. Fix this with current review comments.

@lizan
Copy link

lizan commented Sep 11, 2019

@mandarjog Thanks, that SGTM.

@bianpengyuan
Copy link
Author

@lizan upstream pr created envoyproxy#8205

@bianpengyuan bianpengyuan requested a review from lizan September 11, 2019 03:56
Copy link

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. We can merge this first if it blocks release, and then see how it goes in upstream.

@@ -266,8 +266,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
envoy::type::FractionalPercent random_sampling;
// TODO: Random sampling historically was an integer and default to out of 10,000. We should

Choose a reason for hiding this comment

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

Is the TODO comment still relevant?

@rlenglet rlenglet merged commit 8647faf into istio:release-1.3 Sep 11, 2019
Copy link

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

@bianpengyuan Sorry for introducing the bug - thanks for finding and fixing.

Miss-you pushed a commit to Miss-you/envoy that referenced this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants