-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fault: use FractionalPercent for percent #3978
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
Changes from 3 commits
35dc15a
f105661
4daaac1
4e3a07d
d4a0e76
6652866
f14d3bc
3d56b3f
f197fe2
f98bcc0
c0f9701
ca6fa14
9b1046f
aef3da0
c31bd82
f167d92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ syntax = "proto3"; | |
| package envoy.config.filter.fault.v2; | ||
| option go_package = "v2"; | ||
|
|
||
| import "envoy/type/percent.proto"; | ||
|
|
||
| import "google/protobuf/duration.proto"; | ||
|
|
||
| import "validate/validate.proto"; | ||
|
|
@@ -24,7 +26,11 @@ message FaultDelay { | |
|
|
||
| // An integer between 0-100 indicating the percentage of operations/connection requests | ||
| // on which the delay will be injected. | ||
| uint32 percent = 2 [(validate.rules).uint32.lte = 100]; | ||
| // | ||
| // .. attention:: | ||
| // | ||
| // This field is deprecated and `percentage` should be used instead. | ||
| uint32 percent = 2 [(validate.rules).uint32.lte = 100, deprecated = true]; | ||
|
|
||
| oneof fault_delay_secifier { | ||
| option (validate.required) = true; | ||
|
|
@@ -37,4 +43,7 @@ message FaultDelay { | |
| google.protobuf.Duration fixed_delay = 3 | ||
| [(validate.rules).duration.gt = {}, (gogoproto.stdduration) = true]; | ||
| } | ||
|
|
||
| // The percentage of operations/connection requests on which the delay will be injected. | ||
| envoy.type.FractionalPercent percentage = 4; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you do the same for aborts as well ? |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,19 @@ class Snapshot { | |
| virtual bool featureEnabled(const std::string& key, uint64_t default_value, | ||
| uint64_t random_value) const PURE; | ||
|
|
||
| /** | ||
| * Test if a feature is enabled using the built in random generator and total number of buckets | ||
| * for sampling. | ||
| * @param key supplies the feature key to lookup. | ||
| * @param default_value supplies the default value that will be used if either the feature key | ||
| * does not exist or it is not an integer. | ||
| * @param num_buckets control max number of buckets for sampling. Sampled value will be in a range | ||
| * of [0, num_buckets). | ||
| * @return true if the feature is enabled. | ||
| */ | ||
| virtual bool sampleFeatureEnabled(const std::string& key, uint64_t default_value, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I see why you had to change the name here, but IMO "sample" is a bit strange in the sense that the other featureEnabled() functions also do sampling. A few options here:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattklein123 thanks for reviewing! I've addressed your other comments in f197fe2, and I'm working on this one now.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented option 1 in f98bcc0. |
||
| uint64_t num_buckets) const PURE; | ||
|
|
||
| /** | ||
| * Test if a feature is enabled using a supplied stable random value and total number of buckets | ||
| * for sampling. | ||
|
|
@@ -112,12 +125,12 @@ class Snapshot { | |
| * does not exist or it is not an integer. | ||
| * @param random_value supplies the stable random value to use for determining whether the feature | ||
| * is enabled. | ||
| * @param control max number of buckets for sampling. Sampled value will be in a range of | ||
| * [0, num_buckets). | ||
| * @param num_buckets control max number of buckets for sampling. Sampled value will be in a range | ||
| * of [0, num_buckets). | ||
| * @return true if the feature is enabled. | ||
| */ | ||
| virtual bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, | ||
| uint64_t num_buckets) const PURE; | ||
| virtual bool sampleFeatureEnabled(const std::string& key, uint64_t default_value, | ||
| uint64_t random_value, uint64_t num_buckets) const PURE; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattklein123 how are you guys doing fractional percent with runtimes? IOW, do you need runtime support for fractional percent? If not, all changes to this file can be eliminated
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking again, why is this being renamed ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of a signature conflict between the routines highlighted here. |
||
|
|
||
| /** | ||
| * Fetch raw runtime data based on key. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,6 +255,11 @@ void FilterJson::translateMongoProxy( | |
| delay->set_type(envoy::config::filter::fault::v2::FaultDelay::FIXED); | ||
| delay->set_percent(static_cast<uint32_t>(json_fault->getInteger("percent"))); | ||
| JSON_UTIL_SET_DURATION_FROM_FIELD(*json_fault, *delay, fixed_delay, duration); | ||
|
|
||
| if (json_fault->hasObject("percentage")) { | ||
| const auto json_percentage = json_fault->getObject("percentage"); | ||
| JSON_UTIL_SET_FRACTIONALPERCENT(*json_percentage, *delay, percentage); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -280,6 +285,11 @@ void FilterJson::translateFaultFilter( | |
| delay->set_type(envoy::config::filter::fault::v2::FaultDelay::FIXED); | ||
| delay->set_percent(static_cast<uint32_t>(json_config_delay->getInteger("fixed_delay_percent"))); | ||
| JSON_UTIL_SET_DURATION_FROM_FIELD(*json_config_delay, *delay, fixed_delay, fixed_duration); | ||
|
|
||
| if (json_config_delay->hasObject("fixed_delay_percentage")) { | ||
| const auto json_percentage = json_config_delay->getObject("fixed_delay_percentage"); | ||
| JSON_UTIL_SET_FRACTIONALPERCENT(*json_percentage, *delay, percentage); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don’t need this. This is for translation of legacy config only. |
||
| } | ||
|
|
||
| for (const auto json_header_matcher : json_config.getObjectArray("headers", true)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,8 +146,13 @@ std::string RandomGeneratorImpl::uuid() { | |
| return std::string(uuid, UUID_LENGTH); | ||
| } | ||
|
|
||
| bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, | ||
| uint64_t random_value, uint64_t num_buckets) const { | ||
| bool SnapshotImpl::sampleFeatureEnabled(const std::string& key, uint64_t default_value, | ||
| uint64_t num_buckets) const { | ||
| return sampleFeatureEnabled(key, default_value, generator_.random(), num_buckets); | ||
| } | ||
|
|
||
| bool SnapshotImpl::sampleFeatureEnabled(const std::string& key, uint64_t default_value, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip renaming and overload old function name ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore |
||
| uint64_t random_value, uint64_t num_buckets) const { | ||
| return random_value % num_buckets < std::min(getInteger(key, default_value), num_buckets); | ||
| } | ||
|
|
||
|
|
@@ -165,7 +170,7 @@ bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value | |
|
|
||
| bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, | ||
| uint64_t random_value) const { | ||
| return featureEnabled(key, default_value, random_value, 100); | ||
| return sampleFeatureEnabled(key, default_value, random_value, 100); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two functions are not equivalent.. The stable random value is not same as number of buckets above. Am I missing something here>?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const std::string& SnapshotImpl::get(const std::string& key) const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ namespace HttpFilters { | |
| namespace Fault { | ||
|
|
||
| const std::string FaultFilter::DELAY_PERCENT_KEY = "fault.http.delay.fixed_delay_percent"; | ||
| const std::string FaultFilter::DELAY_PERCENTAGE_KEY = "fault.http.delay.fixed_delay_percentage"; | ||
| const std::string FaultFilter::ABORT_PERCENT_KEY = "fault.http.abort.abort_percent"; | ||
| const std::string FaultFilter::DELAY_DURATION_KEY = "fault.http.delay.fixed_duration_ms"; | ||
| const std::string FaultFilter::ABORT_HTTP_STATUS_KEY = "fault.http.abort.http_status"; | ||
|
|
@@ -129,14 +130,26 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b | |
| } | ||
|
|
||
| bool FaultFilter::isDelayEnabled() { | ||
| bool enabled = config_->runtime().snapshot().featureEnabled(DELAY_PERCENT_KEY, | ||
| fault_settings_->delayPercent()); | ||
|
|
||
| if (!downstream_cluster_delay_percent_key_.empty()) { | ||
| enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_delay_percent_key_, | ||
| bool enabled = false; | ||
| if (fault_settings_->delayPercent() != 0) { | ||
| enabled |= config_->runtime().snapshot().featureEnabled(DELAY_PERCENT_KEY, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not right. Runtime overrides config. If there is no runtime the config value becomes the default. |
||
| fault_settings_->delayPercent()); | ||
| if (!downstream_cluster_delay_percent_key_.empty()) { | ||
| enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_delay_percent_key_, | ||
| fault_settings_->delayPercent()); | ||
| } | ||
| } else if (fault_settings_->delayPercentage().numerator() != 0) { | ||
| enabled |= config_->runtime().snapshot().sampleFeatureEnabled( | ||
| DELAY_PERCENTAGE_KEY, fault_settings_->delayPercentage().numerator(), | ||
| ProtobufPercentHelper::fractionalPercentDenominatorToInt( | ||
| fault_settings_->delayPercentage())); | ||
| if (!downstream_cluster_delay_percent_key_.empty()) { | ||
| enabled |= config_->runtime().snapshot().sampleFeatureEnabled( | ||
| downstream_cluster_delay_percent_key_, fault_settings_->delayPercentage().numerator(), | ||
| ProtobufPercentHelper::fractionalPercentDenominatorToInt( | ||
| fault_settings_->delayPercentage())); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flip the logic. Read new runtime or percentage. If not present read old runtime. |
||
| } | ||
|
|
||
| return enabled; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,17 +47,19 @@ class FaultSettings : public Router::RouteSpecificFilterConfig { | |
| } | ||
| uint64_t abortPercent() const { return abort_percent_; } | ||
| uint64_t delayPercent() const { return fixed_delay_percent_; } | ||
| envoy::type::FractionalPercent delayPercentage() const { return fixed_delay_percentage_; } | ||
| uint64_t delayDuration() const { return fixed_duration_ms_; } | ||
| uint64_t abortCode() const { return http_status_; } | ||
| const std::string& upstreamCluster() const { return upstream_cluster_; } | ||
| const std::unordered_set<std::string>& downstreamNodes() const { return downstream_nodes_; } | ||
|
|
||
| private: | ||
| uint64_t abort_percent_{}; // 0-100 | ||
| uint64_t http_status_{}; // HTTP or gRPC return codes | ||
| uint64_t fixed_delay_percent_{}; // 0-100 | ||
| uint64_t fixed_duration_ms_{}; // in milliseconds | ||
| std::string upstream_cluster_; // restrict faults to specific upstream cluster | ||
| uint64_t abort_percent_{}; // 0-100 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you are doing this for delays, why don't you change the aborts as well to fractional ? It makes the implementation streamlined.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this PR is big in itself, should we do it in another PR?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The abort stuff is small. Confined to http only. If done right, it should be touching same files and not much.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'm on it right now. Figured out it wasn't as big of a change as I thought it would be. |
||
| uint64_t http_status_{}; // HTTP or gRPC return codes | ||
| uint64_t fixed_delay_percent_{}; // 0-100 | ||
| envoy::type::FractionalPercent fixed_delay_percentage_{}; // 0-100 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is a simpler way. Convert both percents to fractional (delay and abort). Then in fault filter you simply have to check for new runtime fields for fractional percentage. If absent check old field. If that’s also absent use the value provided in the config.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to implement the change to abort in a separate PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix comment to indicate that this is fractional |
||
| uint64_t fixed_duration_ms_{}; // in milliseconds | ||
| std::string upstream_cluster_; // restrict faults to specific upstream cluster | ||
| std::vector<Http::HeaderUtility::HeaderData> fault_filter_headers_; | ||
| std::unordered_set<std::string> downstream_nodes_{}; // Inject failures for specific downstream | ||
| }; | ||
|
|
@@ -131,6 +133,7 @@ class FaultFilter : public Http::StreamDecoderFilter { | |
| std::string downstream_cluster_abort_http_status_key_{}; | ||
|
|
||
| const static std::string DELAY_PERCENT_KEY; | ||
| const static std::string DELAY_PERCENTAGE_KEY; | ||
| const static std::string ABORT_PERCENT_KEY; | ||
| const static std::string DELAY_DURATION_KEY; | ||
| const static std::string ABORT_HTTP_STATUS_KEY; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of integer
percentvalue is deprecated. Use fractionalpercentagefield instead