upstream: add failure percentage-based outlier detection #8130
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,12 +222,22 @@ DetectorConfig::DetectorConfig(const envoy::api::v2::cluster::OutlierDetection& | |
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, success_rate_request_volume, 100))), | ||
| success_rate_stdev_factor_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, success_rate_stdev_factor, 1900))), | ||
| failure_percentage_threshold_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, failure_percentage_threshold, 85))), | ||
|
There was a problem hiding this comment. I know this is an established pattern in this file. But it would be good to replace these magic numbers with named constants. Similar to how it is done for Http2Settings here: envoy/include/envoy/http/codec.h Line 230 in d504fde There was a problem hiding this comment. Would it be worth doing in this PR, or a followup? There was a problem hiding this comment. Opened #8221 for this; will update for the new fields once this PR is merged. |
||
| failure_percentage_minimum_hosts_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, failure_percentage_minimum_hosts, 5))), | ||
| failure_percentage_request_volume_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, failure_percentage_request_volume, 50))), | ||
| enforcing_consecutive_5xx_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_consecutive_5xx, 100))), | ||
| enforcing_consecutive_gateway_failure_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_consecutive_gateway_failure, 0))), | ||
| enforcing_success_rate_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_success_rate, 100))), | ||
| enforcing_failure_percentage_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_failure_percentage, 0))), | ||
| enforcing_failure_percentage_local_origin_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_failure_percentage_local_origin, 0))), | ||
| split_external_local_origin_errors_(config.split_external_local_origin_errors()), | ||
| consecutive_local_origin_failure_(static_cast<uint64_t>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, consecutive_local_origin_failure, 5))), | ||
|
|
@@ -355,6 +365,13 @@ bool DetectorImpl::enforceEjection(envoy::data::cluster::v2alpha::OutlierEjectio | |
| return runtime_.snapshot().featureEnabled( | ||
| "outlier_detection.enforcing_local_origin_success_rate", | ||
| config_.enforcingLocalOriginSuccessRate()); | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE: | ||
| return runtime_.snapshot().featureEnabled("outlier_detection.enforcing_failure_percentage", | ||
| config_.enforcingFailurePercentage()); | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE_LOCAL_ORIGIN: | ||
| return runtime_.snapshot().featureEnabled( | ||
| "outlier_detection.enforcing_failure_percentage_local_origin", | ||
| config_.enforcingFailurePercentageLocalOrigin()); | ||
| default: | ||
| // Checked by schema. | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
|
|
@@ -382,6 +399,12 @@ void DetectorImpl::updateEnforcedEjectionStats( | |
| case envoy::data::cluster::v2alpha::OutlierEjectionType::SUCCESS_RATE_LOCAL_ORIGIN: | ||
| stats_.ejections_enforced_local_origin_success_rate_.inc(); | ||
| break; | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE: | ||
| stats_.ejections_enforced_failure_percentage_.inc(); | ||
| break; | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE_LOCAL_ORIGIN: | ||
| stats_.ejections_enforced_local_origin_failure_percentage_.inc(); | ||
| break; | ||
| default: | ||
| // Checked by schema. | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
|
|
@@ -406,6 +429,12 @@ void DetectorImpl::updateDetectedEjectionStats( | |
| case envoy::data::cluster::v2alpha::OutlierEjectionType::SUCCESS_RATE_LOCAL_ORIGIN: | ||
| stats_.ejections_detected_local_origin_success_rate_.inc(); | ||
| break; | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE: | ||
| stats_.ejections_detected_failure_percentage_.inc(); | ||
| break; | ||
| case envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE_LOCAL_ORIGIN: | ||
| stats_.ejections_detected_local_origin_failure_percentage_.inc(); | ||
| break; | ||
| default: | ||
| // Checked by schema. | ||
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
|
|
@@ -609,6 +638,63 @@ void DetectorImpl::processSuccessRateEjections( | |
| } | ||
| } | ||
|
|
||
| void DetectorImpl::processFailurePercentageEjections( | ||
|
There was a problem hiding this comment. can we refactor processSuccessRateEjections for shared utilities? It seems like there's a lot of overlap There was a problem hiding this comment. I took a first pass at this - there's still an unfortunate amount of duplication within the function due to the interactions of minimum hosts/request volume, but I was able to drop a few lines. Do you see any other obvious areas to improve? |
||
| DetectorHostMonitor::SuccessRateMonitorType monitor_type) { | ||
| uint64_t failure_percentage_minimum_hosts = | ||
| runtime_.snapshot().getInteger("outlier_detection.failure_percentage_minimum_hosts", | ||
| config_.failurePercentageMinimumHosts()); | ||
| uint64_t failure_percentage_request_volume = | ||
| runtime_.snapshot().getInteger("outlier_detection.failure_percentage_request_volume", | ||
| config_.failurePercentageRequestVolume()); | ||
| std::vector<HostSuccessRatePair> valid_failure_percentage_hosts; | ||
|
|
||
| // Exit early if there are not enough hosts. | ||
| if (host_monitors_.size() < failure_percentage_minimum_hosts) { | ||
| return; | ||
| } | ||
|
|
||
| // reserve upper bound of vector size to avoid reallocation. | ||
| valid_failure_percentage_hosts.reserve(host_monitors_.size()); | ||
|
|
||
| for (const auto& host : host_monitors_) { | ||
| if (!host.first->healthFlagGet(Host::HealthFlag::FAILED_OUTLIER_CHECK)) { | ||
| absl::optional<double> host_success_rate = | ||
| host.second->getSRMonitor(monitor_type) | ||
| .successRateAccumulator() | ||
| .getSuccessRate(failure_percentage_request_volume); | ||
|
|
||
| if (host_success_rate) { | ||
| valid_failure_percentage_hosts.emplace_back( | ||
| HostSuccessRatePair(host.first, host_success_rate.value())); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!valid_failure_percentage_hosts.empty() && | ||
| valid_failure_percentage_hosts.size() >= failure_percentage_minimum_hosts) { | ||
|
There was a problem hiding this comment. Don't we also need to check to see if host_monitors_.size() >= failure_percentage_minimum_hosts? Ditto for avoiding doing work for success rate if success_rate_minimum_hosts is lower There was a problem hiding this comment. I think this is okay since |
||
| const double failure_percentage_threshold = | ||
| runtime_.snapshot().getInteger("outlier_detection.failure_percentage_threshold", | ||
| config_.failurePercentageThreshold()) / | ||
| 100.0; | ||
|
|
||
| for (const auto& host_success_rate_pair : valid_failure_percentage_hosts) { | ||
| if ((100.0 - host_success_rate_pair.success_rate_) >= failure_percentage_threshold) { | ||
| // We should eject. | ||
|
|
||
| // The ejection type returned by the SuccessRateMonitor's getEjectionType() will be a | ||
| // SUCCESS_RATE type, so we need to figure it out for ourselves. | ||
| const envoy::data::cluster::v2alpha::OutlierEjectionType type = | ||
| (monitor_type == DetectorHostMonitor::SuccessRateMonitorType::ExternalOrigin) | ||
| ? envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE | ||
| : envoy::data::cluster::v2alpha::OutlierEjectionType:: | ||
| FAILURE_PERCENTAGE_LOCAL_ORIGIN; | ||
| updateDetectedEjectionStats(type); | ||
| ejectHost(host_success_rate_pair.host_, type); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void DetectorImpl::onIntervalTimer() { | ||
| MonotonicTime now = time_source_.monotonicTime(); | ||
|
|
||
|
|
@@ -626,6 +712,9 @@ void DetectorImpl::onIntervalTimer() { | |
| processSuccessRateEjections(DetectorHostMonitor::SuccessRateMonitorType::ExternalOrigin); | ||
| processSuccessRateEjections(DetectorHostMonitor::SuccessRateMonitorType::LocalOrigin); | ||
|
|
||
| processFailurePercentageEjections(DetectorHostMonitor::SuccessRateMonitorType::ExternalOrigin); | ||
| processFailurePercentageEjections(DetectorHostMonitor::SuccessRateMonitorType::LocalOrigin); | ||
|
|
||
| armIntervalTimer(); | ||
| } | ||
|
|
||
|
|
@@ -660,6 +749,15 @@ void EventLoggerImpl::logEject(const HostDescriptionConstSharedPtr& host, Detect | |
| detector.successRateEjectionThreshold(monitor_type)); | ||
| event.mutable_eject_success_rate_event()->set_host_success_rate( | ||
| host->outlierDetector().successRate(monitor_type)); | ||
| } else if ((type == envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE) || | ||
| (type == envoy::data::cluster::v2alpha::OutlierEjectionType:: | ||
csssuf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| FAILURE_PERCENTAGE_LOCAL_ORIGIN)) { | ||
| const DetectorHostMonitor::SuccessRateMonitorType monitor_type = | ||
| (type == envoy::data::cluster::v2alpha::OutlierEjectionType::FAILURE_PERCENTAGE) | ||
| ? DetectorHostMonitor::SuccessRateMonitorType::ExternalOrigin | ||
| : DetectorHostMonitor::SuccessRateMonitorType::LocalOrigin; | ||
| event.mutable_eject_failure_percentage_event()->set_host_success_rate( | ||
| host->outlierDetector().successRate(monitor_type)); | ||
| } else { | ||
| event.mutable_eject_consecutive_event(); | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this a reasonable default? I picked this entirely arbitrarily, so I have no attachment to it.
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.
Huh, my instinct would be to say if the type were OutlierEjectionType::FAILURE_PERCENTAGE and has_failure_percentage_threshold were not set, we'd just fail to accept the config.
Looks like that's not terribly consistent with the rest of outlier config, so yeah, I think any well documented value is fine.
Maybe we can tag this for some v4 cleanup (@htuch)
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.
So, just to clarify here - alright to leave it as is now and tag with a
[#next-major-version]comment?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.
definitely fine to leave as-is. Bonus points if you file a tech debt issue, and mention it should be tagged with v4 (I don't think you can add tags)
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.
+1 on filing a tech debt issue on cleaning up all of these messages, agreed it is a big mess as it has evolved quite a bit trying to not break backwards compat. I can tag it correctly once it is opened.
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.
I've filed #8219 for this one.