fix random sample fraction percent #102
Conversation
bianpengyuan
requested review from
rlenglet,
objectiser,
mandarjog,
kyessenov and
douglas-reid
|
@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( | |||
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 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 |
| @@ -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 | |||
There was a problem hiding this comment.
@bianpengyuan Sorry for introducing the bug - thanks for finding and fixing.
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.