outlier: success rate detection by junr03 · Pull Request #546 · envoyproxy/envoy

junr03 · 2017-03-08T15:14:02Z

WIP

To-do:

rshriram · 2017-03-08T16:37:25Z

what does sr mean in this context?

junr03 · 2017-03-08T16:38:07Z

RomanDzhabarov · 2017-03-09T06:17:24Z

+    for (auto host : valid_sr_hosts) {
+      if (host.second < mean - (2 * stdev)) {
+        stats_.ejections_consecutive_5xx_.inc();
+        ejectHost(host.first, EjectionType::SuccessRate);


what's the max #hosts we could potentially eject here? (worry about to eject too many). I'll look into more details, but wondering if you have an answer to this

Yes, there is a max ejection percentage completed in the ejectHost function

RomanDzhabarov · 2017-03-09T06:18:09Z

+struct SRAccumulatorBucket {
+  std::atomic<uint64_t> success_rq_counter_;
+  std::atomic<uint64_t> total_rq_counter_;
+};


nit: new line everywhere in various files

ccaraman · 2017-03-09T18:21:48Z


 class DetectorImpl;

+class SRAccumulatorImpl {


Add comment about the class. I always forget what SR means. Please expand on why we bucket and swap.

ccaraman · 2017-03-09T18:23:53Z

+public:
+  SRAccumulatorImpl()
+      : current_sr_bucket_(new SRAccumulatorBucket()),
+        backup_sr_bucket_(new SRAccumulatorBucket()){};


nit spacing before {}

ccaraman · 2017-03-09T18:29:01Z


 typedef std::shared_ptr<Detector> DetectorPtr;

+struct SRAccumulatorBucket {


I would either move this struct into the class where you are using it or add a comment about what its intents are.

ccaraman · 2017-03-09T18:30:15Z

    checkHostForUneject(host.first, host.second, now);
+
+    // Success Rate Outlier Detection
+    // First swap out the current bucket been written to, to keep data valid


Fix comment.

What do you mean by fix?

Missing full stop.

ccaraman · 2017-03-09T19:05:33Z

+      : current_sr_bucket_(new SRAccumulatorBucket()),
+        backup_sr_bucket_(new SRAccumulatorBucket()){};
+  SRAccumulatorBucket* getCurrentWriter();
+  Optional<double> getSR(uint64_t rq_volume_threshold);


nit: Expand to getSuccessRate and add comment.

I feel that I have used SR everywhere, and expanding it here (while useful, and does not make the name too long) would be confusing.

ccaraman · 2017-03-09T19:21:10Z

+    stdev = std::sqrt(stdev);
+
+    for (auto host : valid_sr_hosts) {
+      if (host.second < mean - (2 * stdev)) {


would we ever want to change how many stdevs away we allow?

Potentially, but I am not sure if we would like to make this configurable. In general for this statistics approach I am more curious to deploy non-enforcing and see how it behaves. My intuition is that mean and stdev is going to be sensitive, but I want to see data.

good to know. I would make the 2 a static const member var that way the name could indicate its use.

I see your point.

ccaraman · 2017-03-09T19:21:26Z

+
+    for (auto host : valid_sr_hosts) {
+      if (host.second < mean - (2 * stdev)) {
+        stats_.ejections_consecutive_5xx_.inc();


wrong stat?

ccaraman · 2017-03-09T19:23:10Z

 }

+SRAccumulatorBucket* SRAccumulatorImpl::getCurrentWriter() {
+  // Right now current_ is being written to and backup_ is not. Flush the backup and swap


add . to the sentence.

RomanDzhabarov · 2017-03-09T22:06:00Z

+                                     config_.rqVolumeThreshold())) {
+
+    // Calculate the statistics (mean, stdev). We are using mean to detect outliers.
+    double mean = sr_sum / sr_data.size();


can you move stats functions into separate math utils or something

RomanDzhabarov · 2017-03-09T22:11:09Z

  uint64_t maxEjectionPercent() { return max_ejection_percent_; }
  uint64_t enforcing() { return enforcing_; }
+  uint64_t significantHostThreshold() { return significant_host_threshold_; }
+  uint64_t rqVolumeThreshold() { return rq_volume_threshold_; }


can you add comments on this as well? e.g. what is significantHostThreshold etc

I'll document this in the docs

RomanDzhabarov · 2017-03-09T22:13:54Z

@@ -209,8 +219,51 @@ void DetectorImpl::onConsecutive5xxWorker(HostPtr host) {

 void DetectorImpl::onIntervalTimer() {


maybe it's a good time to start thinking about small test setup where you can feed upstream hosts params and run ejection on that group of hosts, similar to the idea (https://github.com/lyft/envoy/blob/master/test/common/upstream/load_balancer_simulation_test.cc#L22) of what we are doing to validate how AZ routing works.

With more complicated outlier detection algos we have to have a way to run simulation.

ccaraman · 2017-03-22T00:06:25Z

+  std::vector<double> sr_data;
+  double sr_sum = 0;
+  // reserve upper bound of vector size to avoid reallocation.
+  sr_data.reserve(host_sinks_.size());


nit: can we do the check for hosts_sinks_.size() > significant_host_threshold here too. only reserve if it we need to.

mattklein123 · 2017-03-22T16:43:15Z

+
+  .. _config_cluster_manager_cluster_outlier_detection_enforcing_sr:
+
+  enforcing_sr


s/enforcing_sr/enforcing_success_rate

mattklein123 · 2017-03-22T16:43:51Z

+
+  .. _config_cluster_manager_cluster_outlier_detection_rq_volume_threshold:
+
+  rq_volume_threshold:


s/rq_volume_threshold/request_volume_threshold

mattklein123 · 2017-03-22T16:44:52Z

+
+  .. _config_cluster_manager_cluster_outlier_detection_significant_host_threshold:
+
+  significant_host_threshold


not clear this is related to request volume and success rate. Use a more specific name.

mattklein123 · 2017-03-22T16:45:13Z

+Success Rate
+^^^^^^^^^^^^
+
+Success Rate based outlier ejection aggregates success rate data from every host in a cluster, and at a given


mattklein123 · 2017-03-22T16:45:39Z

+            "minimum" : 0,
+            "exclusiveMinimum" : true
+          },
+          "rq_volume_threshold" : {


request_volume_threshold

mattklein123 · 2017-03-22T16:46:41Z

 }

+void DetectorHostSinkImpl::updateCurrentSRBucket() {
+  sr_accumulator_bucket_.store(sr_accumulator_.getCurrentWriter());


Everywhere in this entire change where you do just SR switch to some variant of "Success rate"

mattklein123 · 2017-03-22T16:49:01Z

+const double Utility::SR_STDEV_FACTOR = 1.9;
+
+double Utility::srEjectionThreshold(double sr_sum, std::vector<double>& sr_data) {
+  double mean = sr_sum / sr_data.size();


Add comment in this function on what all of this math is actually doing, with an example.

mattklein123 · 2017-03-22T16:49:32Z


+const double Utility::SR_STDEV_FACTOR = 1.9;
+
+double Utility::srEjectionThreshold(double sr_sum, std::vector<double>& sr_data) {


const std::vector&

mattklein123 · 2017-03-22T16:50:14Z

    checkHostForUneject(host.first, host.second, now);
+
+    // Success Rate Outlier Detection.
+    // First swap out the current bucket been written to, to keep data valid.


mattklein123 · 2017-03-22T16:50:48Z

+
 void DetectorImpl::onIntervalTimer() {
  SystemTime now = time_source_.currentSystemTime();
+  std::unordered_map<HostPtr, double> valid_sr_hosts;


Move all success rate interval handling into dedicated function

mattklein123 · 2017-03-22T16:59:36Z

+class Utility {
+public:
+  /**
+   * This function returns the Success Rate trheshold for Success Rate outlier detection. If a


mattklein123 · 2017-03-22T17:01:48Z

+ * stats. This implementation has a fixed window size of time, and thus only needs a
+ * bucket to write to, and a bucket to accumulate/run stats over.
+ */
+class SRAccumulatorImpl {


remove Impl, not implementing an interface

mattklein123 · 2017-03-22T17:02:45Z

+      : current_sr_bucket_(new SRAccumulatorBucket()),
+        backup_sr_bucket_(new SRAccumulatorBucket()) {}
+
+  SRAccumulatorBucket* getCurrentWriter();


this is poorly named, as it does the swap also. Make name more descriptive and add comment.

mattklein123 · 2017-03-22T20:50:02Z

Another high level comment: It's unclear to me how we are going to usefully debug this change during rollout. You probably need logging changes, potentially the ability to dump current SR from /clusters output, etc. That needs thinking.

mattklein123

thanks, much easier to understand. some more comments.

mattklein123 · 2017-03-23T00:11:20Z

+const double Utility::SUCCESS_RATE_STDEV_FACTOR = 1.9;
+
+double Utility::successRateEjectionThreshold(double success_rate_sum,
+                                             std::vector<double>& success_rate_data) {


const std::vector& (I think I already commented on this but not sure)

You did comment on this already. I missed it writing the big comment after the function signature.

mattklein123 · 2017-03-23T00:13:21Z

  ejectHost(host, EjectionType::Consecutive5xx);
 }

+const double Utility::SUCCESS_RATE_STDEV_FACTOR = 1.9;


comment, how was this chosen?

mattklein123 · 2017-03-23T00:14:19Z

+  if (host_sinks_.size() >= success_rate_minimum_hosts) {
+    // reserve upper bound of vector size to avoid reallocation.
+    success_rate_data.reserve(host_sinks_.size());
+  }


Do an early return if you don't have enough hosts.

mattklein123 · 2017-03-23T00:15:15Z

+    host.second->updateCurrentSuccessRateBucket();
+
+    // If there are not enough hosts to begin with, don't do the work.
+    if (host_sinks_.size() >= success_rate_minimum_hosts) {


Do an early return if you don't have enough hosts.

mattklein123 · 2017-03-23T00:16:49Z

+}
+
+void DetectorImpl::successRateEjections() {
+  std::unordered_map<HostSharedPtr, double> valid_success_rate_hosts;


You don't actually need map lookup here, an std::list or reserved std::vector containing a struct with host and double will be more efficient.

RomanDzhabarov · 2017-03-23T05:02:39Z

-    for (auto host : valid_success_rate_hosts) {
-      if (host.second < ejection_threshold) {
+        Utility::successRateEjectionThreshold(success_rate_sum, valid_success_rate_hosts);
+    for (auto tuple : valid_success_rate_hosts) {


const auto&.
on a side note, I'd make it more readable with some simple struct introduction with named fields instead of get<1>, get<0>

RomanDzhabarov · 2017-03-23T05:16:48Z

+    return Optional<double>();
+  }
+
+  return Optional<double>(backup_success_rate_bucket_->success_request_counter_ * 100 /


you are losing precision here, should probably sr_counter_ * 100.0 / total_coutner_

RomanDzhabarov · 2017-03-23T05:21:32Z

+   * @return a valid Optional<double> with the success rate. If there were not enough requests, an
+   *         invalid Optional<double> is returned.
+   */
+  Optional<double> getSuccessRate(uint64_t success_rate_request_volume);


param name is incorrect... request_volume_threshold

RomanDzhabarov · 2017-03-23T05:23:31Z

 };

+/**
+ * Utilities for Outlier Detection


nit: Utilities for Outlier Detection.

mattklein123 · 2017-03-23T18:06:44Z

    checkHostForUneject(host.first, host.second, now);
  }

+  successRateEjections();


nit: I would rename processSuccessRateEjections()

mattklein123

looks good after name nit

Jose Nino added 2 commits March 8, 2017 07:09

inital code

e12534a

fix format

2553e6c

Jose Nino added 2 commits March 8, 2017 09:39

add sr ejection counter

65c8c8c

fix

df94dde

junr03 changed the title ~~outlier: sr detection~~ outlier: success rate detection Mar 8, 2017

RomanDzhabarov reviewed Mar 9, 2017

View reviewed changes

ccaraman reviewed Mar 9, 2017

View reviewed changes

Jose Nino added 4 commits March 9, 2017 14:17

update

5cf0a52

Merge branch 'master' into sr-outlier-detection

b952435

update

ab66fa1

add config values

ad408f5

RomanDzhabarov reviewed Mar 9, 2017

View reviewed changes

Jose Nino added 6 commits March 9, 2017 16:14

fix format

4255560

move stats into function

5ec0abe

Merge branch 'master' into sr-outlier-detection

4fde96a

test, config, enforcing, docs

9753ca2

tests

a4e588d

tests

77b407b

update comments

39e75a1

ccaraman reviewed Mar 22, 2017

View reviewed changes

update

c2b00d0

mattklein123 reviewed Mar 22, 2017

View reviewed changes

Jose Nino added 8 commits March 22, 2017 14:38

address most comments

19aa4c3

schema

fd5491e

separation

ae980da

Merge branch 'master' into sr-outlier-detection

e9be3a2

fix tests after merge

b8674dc

fix

b196f43

remove clion include

cd3867a

fix comment

e0f3a9f

mattklein123 reviewed Mar 23, 2017

View reviewed changes

comments

d5750e3

RomanDzhabarov reviewed Mar 23, 2017

View reviewed changes

update comments

607512c

junr03 force-pushed the sr-outlier-detection branch from 4321114 to 607512c Compare March 23, 2017 17:42

fix format

49c0587

mattklein123 reviewed Mar 23, 2017

View reviewed changes

mattklein123 approved these changes Mar 23, 2017

View reviewed changes

name change

05dec22

junr03 merged commit 6fbc857 into master Mar 23, 2017

junr03 deleted the sr-outlier-detection branch March 23, 2017 18:52


		typedef std::shared_ptr<Detector> DetectorPtr;

		struct SRAccumulatorBucket {

		@@ -209,8 +219,51 @@ void DetectorImpl::onConsecutive5xxWorker(HostPtr host) {

		void DetectorImpl::onIntervalTimer() {


		.. _config_cluster_manager_cluster_outlier_detection_enforcing_sr:

		enforcing_sr


		.. _config_cluster_manager_cluster_outlier_detection_rq_volume_threshold:

		rq_volume_threshold:


		.. _config_cluster_manager_cluster_outlier_detection_significant_host_threshold:

		significant_host_threshold


		const double Utility::SR_STDEV_FACTOR = 1.9;

		double Utility::srEjectionThreshold(double sr_sum, std::vector<double>& sr_data) {

Conversation

junr03 commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented Mar 8, 2017

Uh oh!

junr03 commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Mar 8, 2017 •

edited

Loading

ccaraman Mar 9, 2017 •

edited

Loading

junr03 Mar 9, 2017 •

edited

Loading

mattklein123 Mar 23, 2017 •

edited

Loading