fix random sample fraction percent#102
Conversation
bianpengyuan
requested review from
douglas-reid,
kyessenov,
mandarjog,
objectiser and
rlenglet
|
@bianpengyuan i'm assuming this change has been manually validated. |
| 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, see the test that I added.
lizan
left a comment
lizan
left a comment
There was a problem hiding this comment.
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.
|
@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? |
|
Get this reviewed in upstream together, and also, add a test please. |
|
@lizan is there a way to fast-track the Envoy PR review somehow, if we add the tests? |
|
@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. |
source/extensions/filters/network/http_connection_manager/config.cc
Outdated
Show resolved
Hide resolved
|
We will do that, thanks @lizan
|
|
@mandarjog Thanks, that SGTM. |
|
@lizan upstream pr created envoyproxy#8205 |
lizan
left a comment
lizan
left a comment
There was a problem hiding this comment.
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 | |||
objectiser
left a comment
objectiser
left a comment
There was a problem hiding this comment.
@bianpengyuan Sorry for introducing the bug - thanks for finding and fixing.
8 participants
istio/istio#16985
Root cause: https://github.com/envoyproxy/envoy/pull/6986/files#diff-c68454405a6c92916edd63682d33246fL234
random_samplingis aPercent:https://github.com/envoyproxy/envoy/blob/38b926c63f347a70c933e0854ee9f31b1d2e85ce/api/envoy/config/filter/network/http_connection_manager/v3alpha/http_connection_manager.proto#L111
which ranges [0.0, 100.0]. The current code caps random sampling rate at 1%. This fix brings back the percentage numerator rounding to 10000.