-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
fault: use FractionalPercent for percent #3978
Changes from 8 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 |
---|---|---|
|
@@ -11,6 +11,7 @@ Version history | |
`google.api.HttpBody <https://github.com/googleapis/googleapis/blob/master/google/api/httpbody.proto>`_. | ||
* config: v1 disabled by default. v1 support remains available until October via flipping --v2-config-only=false. | ||
* config: v1 disabled by default. v1 support remains available until October via setting :option:`--allow-deprecated-v1-api`. | ||
* fault: added support for :ref:`fractional percentages <envoy_api_field_config.filter.fault.v2.FaultDelay.percentage>`. | ||
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. Please note the deprecations in deprecated.md |
||
* health check: added support for :ref:`custom health check <envoy_api_field_core.HealthCheck.custom_health_check>`. | ||
* health check: added support for :ref:`specifying jitter as a percentage <envoy_api_field_core.HealthCheck.interval_jitter_percent>`. | ||
* health_check: added support for :ref:`health check event logging <arch_overview_health_check_logging>`. | ||
|
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, | ||
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:
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. 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; | ||
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 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 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 ? 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 |
---|---|---|
|
@@ -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, | ||
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 ? 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); | ||
} | ||
|
||
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>? 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 |
---|---|---|
|
@@ -34,12 +34,22 @@ const std::string FaultFilter::ABORT_HTTP_STATUS_KEY = "fault.http.abort.http_st | |
FaultSettings::FaultSettings(const envoy::config::filter::http::fault::v2::HTTPFault& fault) { | ||
|
||
if (fault.has_abort()) { | ||
abort_percent_ = fault.abort().percent(); | ||
if (fault.abort().has_percentage()) { | ||
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. extract to helper function for duplicated code here and below. |
||
abort_percentage_ = fault.abort().percentage(); | ||
} else { | ||
abort_percentage_.set_numerator(fault.abort().percent()); | ||
abort_percentage_.set_denominator(envoy::type::FractionalPercent::HUNDRED); | ||
} | ||
http_status_ = fault.abort().http_status(); | ||
} | ||
|
||
if (fault.has_delay()) { | ||
fixed_delay_percent_ = fault.delay().percent(); | ||
if (fault.delay().has_percentage()) { | ||
fixed_delay_percentage_ = fault.delay().percentage(); | ||
} else { | ||
fixed_delay_percentage_.set_numerator(fault.delay().percent()); | ||
fixed_delay_percentage_.set_denominator(envoy::type::FractionalPercent::HUNDRED); | ||
} | ||
const auto& delay = fault.delay(); | ||
fixed_duration_ms_ = PROTOBUF_GET_MS_OR_DEFAULT(delay, fixed_delay, 0); | ||
} | ||
|
@@ -129,26 +139,28 @@ Http::FilterHeadersStatus FaultFilter::decodeHeaders(Http::HeaderMap& headers, b | |
} | ||
|
||
bool FaultFilter::isDelayEnabled() { | ||
bool enabled = config_->runtime().snapshot().featureEnabled(DELAY_PERCENT_KEY, | ||
fault_settings_->delayPercent()); | ||
|
||
bool enabled = config_->runtime().snapshot().sampleFeatureEnabled( | ||
DELAY_PERCENT_KEY, fault_settings_->delayPercentage().numerator(), | ||
ProtobufPercentHelper::fractionalPercentDenominatorToInt(fault_settings_->delayPercentage())); | ||
if (!downstream_cluster_delay_percent_key_.empty()) { | ||
enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_delay_percent_key_, | ||
fault_settings_->delayPercent()); | ||
enabled |= config_->runtime().snapshot().sampleFeatureEnabled( | ||
downstream_cluster_delay_percent_key_, fault_settings_->delayPercentage().numerator(), | ||
ProtobufPercentHelper::fractionalPercentDenominatorToInt( | ||
fault_settings_->delayPercentage())); | ||
} | ||
|
||
return enabled; | ||
} | ||
|
||
bool FaultFilter::isAbortEnabled() { | ||
bool enabled = config_->runtime().snapshot().featureEnabled(ABORT_PERCENT_KEY, | ||
fault_settings_->abortPercent()); | ||
|
||
bool enabled = config_->runtime().snapshot().sampleFeatureEnabled( | ||
ABORT_PERCENT_KEY, fault_settings_->abortPercentage().numerator(), | ||
ProtobufPercentHelper::fractionalPercentDenominatorToInt(fault_settings_->abortPercentage())); | ||
if (!downstream_cluster_abort_percent_key_.empty()) { | ||
enabled |= config_->runtime().snapshot().featureEnabled(downstream_cluster_abort_percent_key_, | ||
fault_settings_->abortPercent()); | ||
enabled |= config_->runtime().snapshot().sampleFeatureEnabled( | ||
downstream_cluster_abort_percent_key_, fault_settings_->abortPercentage().numerator(), | ||
ProtobufPercentHelper::fractionalPercentDenominatorToInt( | ||
fault_settings_->abortPercentage())); | ||
} | ||
|
||
return enabled; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,19 +45,19 @@ class FaultSettings : public Router::RouteSpecificFilterConfig { | |
const std::vector<Http::HeaderUtility::HeaderData>& filterHeaders() const { | ||
return fault_filter_headers_; | ||
} | ||
uint64_t abortPercent() const { return abort_percent_; } | ||
uint64_t delayPercent() const { return fixed_delay_percent_; } | ||
envoy::type::FractionalPercent abortPercentage() const { return abort_percentage_; } | ||
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 | ||
envoy::type::FractionalPercent abort_percentage_{}; // 0.0-100.0 | ||
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.
|
||
uint64_t http_status_{}; // HTTP or gRPC return codes | ||
envoy::type::FractionalPercent fixed_delay_percentage_{}; // 0.0-100.0 | ||
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 | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,13 +102,19 @@ typedef std::shared_ptr<AccessLog> AccessLogSharedPtr; | |
class FaultConfig { | ||
public: | ||
FaultConfig(const envoy::config::filter::fault::v2::FaultDelay& fault_config) | ||
: delay_percent_(fault_config.percent()), | ||
duration_ms_(PROTOBUF_GET_MS_REQUIRED(fault_config, fixed_delay)) {} | ||
uint32_t delayPercent() const { return delay_percent_; } | ||
: duration_ms_(PROTOBUF_GET_MS_REQUIRED(fault_config, fixed_delay)) { | ||
if (fault_config.has_percentage()) { | ||
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 utility function extracted above |
||
delay_percentage_ = fault_config.percentage(); | ||
} else { | ||
delay_percentage_.set_numerator(static_cast<uint32_t>(fault_config.percent())); | ||
delay_percentage_.set_denominator(envoy::type::FractionalPercent::HUNDRED); | ||
} | ||
} | ||
envoy::type::FractionalPercent delayPercentage() const { return delay_percentage_; } | ||
uint64_t delayDuration() const { return duration_ms_; } | ||
|
||
private: | ||
const uint32_t delay_percent_; | ||
envoy::type::FractionalPercent delay_percentage_; | ||
const uint64_t duration_ms_; | ||
}; | ||
|
||
|
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.
Can you do the same for aborts as well ?