Preview decision and remove individual keys #146

jmfrank63 · 2022-09-13T14:21:25Z

Hi Andreas,

This might be a weird PR, but I have a use case for it. I am rate-limiting login failures, and thus would like to know in advance before I send a request to the rather costly authentication, what the decision would be, so in case of a failure I reject right away.
I considered wrapping first, but that would have meant to deal with the whole rate limiter for an individual decision.
Also, I wanted to have the ability to remove a key from the rate limiter, if some condition happens. The use case here is a user login. If it fails too often it gets rate limited, but if it succeeds it is reset.

I am a rather average coder, so I basically duplicated the code. But I am happy to take suggestions. Two things come to mind:
Call check_key for peek_check_key instead of duplicating the code. Use fetch_and_update instead of the compare_exchange_weak while loop.

I added more tests, as I intend to use this code in production. Please let me know what you think.

antifuchs · 2022-09-15T19:34:23Z

I meant to do this, as well! Thanks so much for the PR (and sorry for not getting to reviewing it&the previous PR earlier - work combined with some sickness). I think having a peek method is great & promised to write one in some other issues (#131 at least!), so this is much appreciated (:

codecov · 2022-09-15T19:42:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83345b1) 98.15% compared to head (f715e04) 98.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   98.15%   98.60%   +0.45%     
==========================================
  Files          31       31              
  Lines        2225     2949     +724     
==========================================
+ Hits         2184     2908     +724     
  Misses         41       41

Files	Coverage Δ
governor/src/clock/quanta.rs	`98.50% <100.00%> (-0.03%)`	⬇️
governor/src/clock/with_std.rs	`100.00% <100.00%> (ø)`
governor/src/errors.rs	`100.00% <100.00%> (ø)`
governor/src/gcra.rs	`100.00% <100.00%> (ø)`
governor/src/jitter.rs	`100.00% <100.00%> (ø)`
governor/src/state.rs	`100.00% <100.00%> (ø)`
governor/src/state/direct.rs	`100.00% <100.00%> (ø)`
governor/src/state/direct/future.rs	`100.00% <100.00%> (ø)`
governor/src/state/in_memory.rs	`100.00% <100.00%> (ø)`
governor/src/state/keyed.rs	`100.00% <100.00%> (ø)`
... and 8 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

antifuchs

This looks right! I'd like to get #144 in first, so that this looks a bit cleaner with the clippy fix-ups, but overall this is great!

governor/src/gcra.rs

jmfrank63 · 2022-09-21T18:12:48Z

I rebased but since I first had tried to resolve the conflicts via the web GUI (an awful idea) rebasing was a bit messy. I hope I didn't break anything, at least the test suite passed.

jmfrank63 · 2022-12-08T12:54:09Z

@antifuchs Hi Andreas, I think I could need some help here. My tests are failing if no standard library is used. Could you please give me a hint on what to do?

jmfrank63 · 2022-12-11T04:34:36Z

@antifuchs OK, I found the issue and fixed it. I added more tests to get the pipeline "green" as well as increasing code coverage slightly.
Since we are using the preview in our code, we currently have a fork hosted, which we have to keep in sync. Therefore, I have a strong interest to get back to an external reference of the crate.
Please let me know if there is anything else for me to do.

jmfrank63 · 2023-01-13T09:45:06Z

@antifuchs Happy new year, Andreas.
I just wanted to ask if there is anything else I could do. I don't seem to have any control over bors, so if you could have a look, that would be great.

Thanks, Johannes

antifuchs · 2023-03-19T23:40:04Z

Sorry for taking so long with this. I'm coming into a good bit of spare time soon, so I'll review this in depth ASAP!

Nutomic · 2023-07-04T14:43:49Z

I also need this. Would be great if it could be merged soon, because there seems to be no other Rust crate with this functionality.

jmfrank63 · 2023-08-06T08:56:29Z

Hi @antifuchs,

I was just revisiting this, it does not seem to need a rebase. Is there any other work you want me to do to get this merged?
I also saw, others seem to be interested in this, too.

Thanks, Johannes

antifuchs

This does look great! Besides a rebase, I highlighted a few items that still need work (mostly coverage-related, which will prevent a merge... but also missing docs on public methods).

I have owed you this review for a long time, thanks for picking up work on this change again!

antifuchs · 2023-08-06T15:22:35Z

governor/src/state/in_memory.rs

+                }
+                Err(next_prev) => prev = next_prev,
+            }
+            decision = f(NonZeroU64::new(prev).map(|n| n.get().into()));


Looks like codecov thinks L65-L67 aren't exercised in tests; would be great if there was a way to test those too.

I think I got this one now covered. Interestingly, now that codecov is succeeding, I cannot find the individual cover report any more. Should there be anything left, please let me know.

antifuchs · 2023-08-06T15:23:12Z

governor/src/state/in_memory.rs

+    }
+    fn reset(&self, _key: &Self::Key) {
+        self.0.store(0, Ordering::Release);
+    }


Same on coverage of the reset function - I think it'd be great if this could be called in tests at least (:

Same here, now the test succeeded, I can't find the reports on individual lines. I hope I got everything covered. Happy to amend.

antifuchs · 2023-08-06T15:25:08Z

governor/src/state.rs

@@ -46,6 +46,13 @@ pub trait StateStore {
    fn measure_and_replace<T, F, E>(&self, key: &Self::Key, f: F) -> Result<T, E>
    where
        F: Fn(Option<Nanos>) -> Result<(T, Nanos), E>;
+    /// `measure_and_peek` does the same as `measure_and_replace` except


formatting nit:

Suggested change

/// `measure_and_peek` does the same as `measure_and_replace` except

/// `measure_and_peek` does the same as `measure_and_replace` except

Correct, added the line.

antifuchs · 2023-08-06T15:25:59Z

governor/src/state/keyed.rs

@@ -98,6 +98,15 @@ where
        )
    }

+    pub fn peek_key(&self, key: &K) -> Result<MW::PositiveOutcome, MW::NegativeOutcome> {


Please add a documentation comment on this public function.

Docstring added. Docs are often a matter of taste. Please let me know if you want me to change anything.

antifuchs · 2023-08-06T15:26:08Z

governor/src/state/keyed.rs

+            .peek_test::<K, C::Instant, S, MW>(self.start, key, &self.state, self.clock.now())
+    }
+
+    pub fn reset_key(&self, key: &K) {


public function: needs a doc comment

Same here, docstring added.

antifuchs · 2023-08-06T15:26:42Z

governor/src/state/direct.rs

@@ -80,6 +80,15 @@ where
        )
    }

+    pub fn peek(&self) -> Result<MW::PositiveOutcome, MW::NegativeOutcome> {


public function: needs a doc comment

Correct, docstring added.

antifuchs · 2023-08-06T15:26:46Z

governor/src/state/direct.rs

@@ -107,6 +116,19 @@ where
                self.clock.now(),
            )
    }
+
+    pub fn peek_n(


public function: needs a doc comment

antifuchs · 2023-08-06T15:28:47Z

governor/tests/future.rs

-const MAX_TEST_RUN_DURATION: Duration = Duration::from_micros(200);
+const PROCEEDS_DURATION_MAX_MICROS: u64 = 250;
+const PAUSES_DURATION_MIN_MILLIS: u64 = 100;
+const MULTIPLE_DURATION_MIN_MILLIS: u64 = 8;


I believe the current main branch has a fix to these on it already (think it was #157) - so I'm not sure if these extra constants are needed as workarounds to the flakey tests anymore.

I removed them for now. The tests for time duration are actually pointless. All we need is to make sure the test succeeds. I have therefore removed all tests against times as this is really flaky.
Please let me know if you don't like it, I will undo it.

antifuchs · 2023-08-06T15:35:51Z

governor/tests/keyed_dashmap.rs

+        assert!(
+            lb.check_key_n(key, nonzero!(2u32)).is_ok(),
+            "Now: {:?}",
+            clock.now()
+        );


codecov warns that lines in this multi-line assert aren't covered which... yeah ):

I think the easiest way to get rid of that is to reformat the condition you're checking in a let-bound variable and then assert on the variable.

Suggested change

assert!(

lb.check_key_n(key, nonzero!(2u32)).is_ok(),

"Now: {:?}",

clock.now()

);

let res = lb.check_key_n(key, nonzero!(2u32));

assert!(res.is_ok(), "Now: {:?}", clock.now());

(would be neat if we had assert_matches for these checks, but that's not stable yet)

I think I got a solution. What is actually not covered here is the case the check_n_key fails.

antifuchs · 2023-08-06T15:36:18Z

governor/tests/keyed_dashmap.rs

+            lb.check_key_n(key, nonzero!(2u32)).is_ok(),
+            "Now: {:?}",
+            clock.now()
+        );


ditto on coverage of that multiline-assert.

Yeah, took a while, but I think I got it now. If I missed anything, please let me know.

jmfrank63 · 2023-08-09T17:12:28Z

This does look great! Besides a rebase, I highlighted a few items that still need work (mostly coverage-related, which will prevent a merge... but also missing docs on public methods).

I have owed you this review for a long time, thanks for picking up work on this change again!

@antifuchs Hi Andreas,

thank you so much for this opportunity. I think I have addressed all the issues. I also factored out two tests in their own functions and added one. For the collision, I changed the current test, it now succeeds really fast, I had successes with 4 retries. I have also removed the flaky timing tests on the proceed functions, all that is necessary is the fact that they proceed, not how fast.
If you don't like anything, just let me know, I'll change this.
If you know how to get the coverage report, like when it is failing, give me a hint. I couldn't find it :-)
Thanks again Johannes

jmfrank63 · 2023-08-10T12:16:16Z

Hi Andreas,

Finally, full coverage, since I found the coverage report again. I also found that the M1 bug is now fixed, so I activated this test as well. I think you will decide on the version, so I left the version untouched.

jmfrank63 · 2023-08-22T00:17:00Z

@antifuchs Hi Andreas,
I am not sure if I got all your concerns. The long time in between visiting this accumulated a lot of changes. I hope I have addressed all of them. While I am not at the company I used the code for actively in production, I care about my former colleagues. Currently, they have to maintain the fork. It would be nice if they could just return to a simple cargo dependency.
If I should do additional work on this, let me know. Squashing might be an option, but this often makes commenting difficult, so I only squash on request as merging does squash anyway.

jmfrank63 · 2023-10-09T21:20:02Z

@antifuchs
Hi Andreas,
I saw you are preparing for the next release. I would be happy if my PR could make it in there. I have increased my code coverage and resolved all issues I could find. All tests passed.
Please let me know if I should address anything else.
Thanks, Johannes

codecov · 2023-12-30T06:43:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f0cb09c) 98.25% compared to head (ae82c95) 98.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   98.25%   98.71%   +0.45%     
==========================================
  Files          31       31              
  Lines        2182     2957     +775     
==========================================
+ Hits         2144     2919     +775     
  Misses         38       38

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jmfrank63 · 2024-01-01T05:47:01Z

@antifuchs Dear Andreas, Happy new year 2024.
I've rebased to reflect the latest changes, but I cannot do this forever. I am not invested into this any more as I have left the company, but for my colleagues it is painful to keep a local copy of an earlier version. I have added additional tests to increase coverage and have, to my best ability, addressed all concerns.
Therefore, I kindly ask you to consider merging now, so we can close this PR. You might want to review again, that's fine, I will try to address additional concerns ASAP.

Thanks, Johannes

Implementing peeking Add more peek functions Fix failing tests Implemented peek Fix n key peek function Added peek tests for n advances Implemented reset Added reset test for dashmap stable formatting fix failing no)_std tests Fix formatting Clippy lints Add test for peek Add tests Add tests Add dashmap tests assert coverage test Address coverage formatting Move reset from unit to bool return type Change to nested results Format code direct tests codecov Codecov wip Improve codecov and tests Codecov wip formatting Codecov wip formatting Improve runtime Add rand dep Test peek formatting fix compiling fix std lib Improve tests formattings non std import removed Improve reset tests Adressed comments Added tests Call measure and peek in collision Added peek test for codecov Include test for M1 since now fixed Remove clock default impl Add coverage code Silence clippy Increase coverage Clippy fixes Trigger pipeline Rebase upstream Add race condition test formatting limit test to std Trigger pipeline Retrigger pipeline trigger compare_exchange_weak failure Improve comment Add comments Reduce sleep Add tests for clone and default Add clone and debug formatting Fix debug test

antifuchs approved these changes Sep 15, 2022

View reviewed changes

governor/src/gcra.rs Show resolved Hide resolved

jmfrank63 force-pushed the expose-statestore branch from aff84d7 to f54bacf Compare September 21, 2022 18:09

jmfrank63 force-pushed the expose-statestore branch from f54bacf to 05aaec4 Compare December 8, 2022 12:44

jmfrank63 force-pushed the expose-statestore branch from 54c1182 to ad393eb Compare December 11, 2022 04:44

Nutomic mentioned this pull request Jul 10, 2023

Add failure rate limit for sends LemmyNet/activitypub-federation-rust#60

Closed

antifuchs reviewed Aug 6, 2023

View reviewed changes

jmfrank63 force-pushed the expose-statestore branch from ad393eb to 92f4dfa Compare August 8, 2023 22:17

jmfrank63 force-pushed the expose-statestore branch from 0b05282 to badcd81 Compare November 11, 2023 06:15

jmfrank63 force-pushed the expose-statestore branch from 28928b6 to 7a28bb7 Compare December 30, 2023 06:41

jmfrank63 force-pushed the expose-statestore branch from aa4dfcd to 5a10f34 Compare January 28, 2024 05:10

jmfrank63 force-pushed the expose-statestore branch from 5a10f34 to ae82c95 Compare January 28, 2024 05:13

	/// `measure_and_peek` does the same as `measure_and_replace` except

	/// `measure_and_peek` does the same as `measure_and_replace` except

Preview decision and remove individual keys #146

Are you sure you want to change the base?

Preview decision and remove individual keys #146

Conversation

jmfrank63 commented Sep 13, 2022

antifuchs commented Sep 15, 2022

codecov bot commented Sep 15, 2022 • edited Loading

Codecov Report

antifuchs left a comment

Choose a reason for hiding this comment

jmfrank63 commented Sep 21, 2022 • edited Loading

jmfrank63 commented Dec 8, 2022

jmfrank63 commented Dec 11, 2022

jmfrank63 commented Jan 13, 2023

antifuchs commented Mar 19, 2023

Nutomic commented Jul 4, 2023 • edited Loading

jmfrank63 commented Aug 6, 2023

antifuchs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmfrank63 Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmfrank63 commented Aug 9, 2023

jmfrank63 commented Aug 10, 2023

jmfrank63 commented Aug 22, 2023 • edited Loading

jmfrank63 commented Oct 9, 2023

codecov bot commented Dec 30, 2023 • edited Loading

Codecov Report

jmfrank63 commented Jan 1, 2024

codecov bot commented Sep 15, 2022 •

edited

Loading

jmfrank63 commented Sep 21, 2022 •

edited

Loading

Nutomic commented Jul 4, 2023 •

edited

Loading

jmfrank63 Aug 9, 2023 •

edited

Loading

jmfrank63 commented Aug 22, 2023 •

edited

Loading

codecov bot commented Dec 30, 2023 •

edited

Loading