Sliding window implementation for PID controller by AbdulRahmanAlHamali · Pull Request #874 · Shopify/semian

AbdulRahmanAlHamali · 2025-11-15T13:52:19Z

Our tests have shown the discrete window implementation to be too janky, with rejection rate dropping too aggressively. Sliding window provides better smoothness.

fixes https://github.com/Shopify/resiliency/issues/6615

kris-gaudel · 2025-11-17T14:52:07Z

lib/semian/timestamped_sliding_window.rb

@@ -0,0 +1,97 @@
+# frozen_string_literal: true


Is there a reason simple_sliding_window.rb wouldn't work here?

Or maybe this could inherit from it

hmmm, I'll look at its implementation, but we'll have to use it in a slightly different way: the classic circuit breaker only slides the window when it receives a new response, but we want to slide it continuously every second. So it will probably need some tweaking

AbdulRahmanAlHamali · 2025-11-19T19:45:33Z

lib/semian.rb

      initial_history_duration: 900, # 15 minutes of initial history for p90 calculation
      initial_error_rate: options[:initial_error_rate] || 0.01, # 1% error rate for initial p90 calculation
-      thread_safe: Semian.thread_safe?,
+      implementation: implementation(**options),


Not exactly part of the PR, but just making the implementation more consistent with the rest of the repo, where they define an implementation and it's either "Simple" or "ThreadSafe"

AbdulRahmanAlHamali · 2025-11-19T19:46:17Z

lib/semian/pid_controller.rb

-
-      @last_p_value = 0.0
-    end
+  module Simple


Tip: set "Hide Whitespaces" to true while reviewing this file

AbdulRahmanAlHamali · 2025-11-19T19:48:26Z

lib/semian/pid_controller.rb

+          initial_value: initial_error_rate,
+        )
+
+        @errors = implementation::SlidingWindow.new


I could've technically put these three into 1 array with entries looking like:

{type:, timestamp}

But this is more efficient since we later have to scan the array to decide how many errors vs. successes, while this way we could simply look at sizes

AbdulRahmanAlHamali · 2025-11-19T19:49:46Z

lib/semian/pid_controller.rb

+        # Clean up old observations
+        current_timestamp = current_time
+        cutoff_time = current_timestamp - @window_size
+        @errors.reject! { |timestamp| timestamp < cutoff_time }


this could be implemented more efficiently, since we know entries are ordered by time. So if found to be too slow, we can optimize

AbdulRahmanAlHamali · 2025-11-19T19:51:25Z

lib/semian/simple_sliding_window.rb


    def push(value)
-      resize_to(@max_size - 1) # make room
+      resize_to(@max_size - 1) unless @max_size.nil? # make room


I think max_size makes sense for the classic, because we know exactly how many requests we want to store before opening the circuit. For adaptive, we don't know that, so I elected to make them unlimited.

This should be ok, since we don't expect to see too many requests to a single dependency. But we could add an optional max_size later if we see issues

I agree it might not be as concrete as the classic, but just thinking out loud it would be good to have some limit to prevent unbound growth

agreed. Do you have a number in mind? Or should we just put some large value, e.g. 1000 or something?

Ideally we should choose a setting that increases/decreases with window size. I'm guessing 100 x window_size. So for window_size = 10, that would be a 1000.

AbdulRahmanAlHamali · 2025-11-19T19:52:26Z

test/pid_controller_test.rb

-
-    assert_equal(0, @controller.metrics[:rejection_rate])
-  end
+  # def test_update_flow


These tests have to be fixed. However, in the next PR, I'm going to simplify the controller, from a PID controller to a simple P controller as discussed. So I'm not going to invest in this now

Aguasvivas22 · 2025-11-21T18:08:14Z

lib/semian/adaptive_circuit_breaker.rb


    def wait_for_window
-      Kernel.sleep(@window_size)
+      Kernel.sleep(@sliding_interval)


Shouldn't this still be the window size?

I don't think so, if we're sliding every second, we would want to wake the thread up every second, but then look back at the entire window size

kris-gaudel · 2025-11-17T14:52:36Z

lib/semian/timestamped_sliding_window.rb

@@ -0,0 +1,97 @@
+# frozen_string_literal: true


Or maybe this could inherit from it

kris-gaudel · 2025-11-25T19:58:05Z

lib/semian/pid_controller.rb

+          observations_per_minute: 60 / sliding_interval,
+        )
+
+        @errors = implementation::SlidingWindow.new(max_size: 100 * window_size)


What is the meaning of multiplying by 100?

basically I'm allowing up to a 100 requests per second to be stored. So a 1000 requests in a 10-second time window.

It's an arbitrary limit. Just not to keep things unbound

lib/semian/pid_controller.rb

Aguasvivas22

lgtm

anagayan · 2025-11-26T17:11:36Z

lib/semian/pid_controller.rb

-    def calculate_window_error_rate
-      total_requests = @current_window_requests[:success] + @current_window_requests[:error]
-      return 0.0 if total_requests == 0
+        total_requests = @successes.size + @errors.size


Why doesn't total requests include the rejections? I feel like there was a reason for this, but I'm blanking on it

you want to find the error rate in the requests that did actually make it to the backend

AbdulRahmanAlHamali changed the base branch from main to kris-gaudel/ses-replace-p2 November 15, 2025 13:52

AbdulRahmanAlHamali force-pushed the sliding-implementation branch from 7bcf49c to 4a3b0de Compare November 15, 2025 13:53

kris-gaudel reviewed Nov 17, 2025

View reviewed changes

AbdulRahmanAlHamali force-pushed the sliding-implementation branch 4 times, most recently from ce2ef9f to 1f6fa64 Compare November 19, 2025 19:43

AbdulRahmanAlHamali commented Nov 19, 2025

View reviewed changes

AbdulRahmanAlHamali changed the title ~~Sliding implementation~~ Sliding window implementation for PID controller Nov 19, 2025

AbdulRahmanAlHamali marked this pull request as ready for review November 19, 2025 19:54

AbdulRahmanAlHamali force-pushed the sliding-implementation branch 3 times, most recently from 29fc9e8 to a3130b9 Compare November 20, 2025 14:48

Base automatically changed from kris-gaudel/ses-replace-p2 to pid-take-2 November 20, 2025 16:24

AbdulRahmanAlHamali force-pushed the sliding-implementation branch 7 times, most recently from 8dfcc48 to 20299bd Compare November 20, 2025 22:14

Aguasvivas22 reviewed Nov 21, 2025

View reviewed changes

AbdulRahmanAlHamali added 7 commits November 25, 2025 18:54

switch algorithm to a sliding window instead of discrete

7fdf705

clean up AI slop PR

d51ed38

provide observations per minute to the smoother from the pid controller

28414b4

fix tests

2fea696

remove unused smoother in setup

1a06349

fix alpha value

5039ee4

fix tests and run experiments

d1f940a

AbdulRahmanAlHamali force-pushed the sliding-implementation branch from 20299bd to 72d0208 Compare November 25, 2025 19:22

AbdulRahmanAlHamali requested review from Aguasvivas22, anagayan and kris-gaudel November 25, 2025 19:44

kris-gaudel reviewed Nov 25, 2025

View reviewed changes

lib/semian/pid_controller.rb Show resolved Hide resolved

kris-gaudel reviewed Nov 25, 2025

View reviewed changes

lib/semian/pid_controller.rb Show resolved Hide resolved

kris-gaudel self-requested a review November 26, 2025 04:31

kris-gaudel approved these changes Nov 26, 2025

View reviewed changes

Aguasvivas22 mentioned this pull request Nov 26, 2025

Add defensiveness coefficient, and simplify the controller into a P-only controller #875

Closed

fix max size for sliding window and rerun experiments

22cd79b

AbdulRahmanAlHamali force-pushed the sliding-implementation branch from 72d0208 to 22cd79b Compare November 26, 2025 16:47

Aguasvivas22 approved these changes Nov 26, 2025

View reviewed changes

anagayan reviewed Nov 26, 2025

View reviewed changes

AbdulRahmanAlHamali merged commit a7d7a57 into pid-take-2 Nov 26, 2025
32 checks passed

AbdulRahmanAlHamali deleted the sliding-implementation branch November 26, 2025 18:16

Conversation

AbdulRahmanAlHamali commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Aguasvivas22 left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AbdulRahmanAlHamali commented Nov 15, 2025 •

edited

Loading