Skip to content
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

Preview decision and remove individual keys #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmfrank63
Copy link

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
Copy link
Collaborator

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
Copy link

codecov bot commented Sep 15, 2022

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.

Copy link
Collaborator

@antifuchs antifuchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@jmfrank63
Copy link
Author

jmfrank63 commented Sep 21, 2022

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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Collaborator

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
Copy link

Nutomic commented Jul 4, 2023

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
Copy link
Author

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

Copy link
Collaborator

@antifuchs antifuchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@jmfrank63 jmfrank63 Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, added the line.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a documentation comment on this public function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

pub fn reset_key(&self, key: &K) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function: needs a doc comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, docstring added.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function: needs a doc comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, docstring added.

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

pub fn peek_n(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function: needs a doc comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also done.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 57 to 61
assert!(
lb.check_key_n(key, nonzero!(2u32)).is_ok(),
"Now: {:?}",
clock.now()
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto on coverage of that multiline-assert.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jmfrank63
Copy link
Author

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
Copy link
Author

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
Copy link
Author

jmfrank63 commented Aug 22, 2023

@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
Copy link
Author

@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

Copy link

codecov bot commented Dec 30, 2023

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
Copy link
Author

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants