[CBR-179] Bouncing and throttling by parsonsmatt · Pull Request #3431 · input-output-hk/cardano-sl

parsonsmatt · 2018-08-17T15:49:20Z

Description

This PR adds throttling capability to the Wallet API as well as the ability to load the configuration from the file.

Note: This requires the configuration.yaml file be modified to include the wallet section. We should figure out how that affects clients.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-179

Type of change

[~] 🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
[~] 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
[~] 🔨 New or improved tests for existing code
[~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

I have not added new tests for this feature. The sort of test we could implement to verify throttling works is to spam the server with a bunch of requests and verify that some of them are 429. However, the test-suite for the library already does this, so we'd be replicating the work.

Existing tests already catch parsing the config format.

QA Steps

Screenshots (if available)

KtorZ

LGTM 👍

KtorZ · 2018-09-08T06:54:10Z

Conflicts come from the split recently done between the legacy wallet and the new one.
I'll try to solve them and get this merged, then possibly issue a PR to cover throttling with the new wallet.

- Format message as JSend, include retry information - Use the API error mechanism so it can be caught more easily on client side - Add to sample, test

This function allows you to uniformly add an error handler to each endpoint of the WalletClient type. This will be used in an upcoming commit to provide the throttling retry logic.

This should prevent the failures from throttling to occur in integration testing until we can have file-level configuration of the integration test.

KtorZ · 2018-09-08T10:00:18Z

Okay, I squashed a bunch of stuff together while rebasing and resolving conflicts. Maaaaaaan, that was something. We really need to work on our git practices 😢 ...

Also, I've made one slight change instead of solving 18 times the same conflict. I've defined a higher abstraction natMapClient which can be used to define hoistClient and mapClientErrors, instead of having to do this plumbing twice in both functions.

-- | Natural transformation + mapping on a 'WalletClient'
natMapClient
    :: (forall x. m x -> n x)
    -> (forall x. n (Either ClientError x) -> n (Either ClientError x))
    -> WalletClient m
    -> WalletClient n
natMapClient phi f wc = WalletClient
    { getAddressIndexPaginated =
        \x -> f . phi . getAddressIndexPaginated wc x
     -- ... etc

-- | Run the given natural transformation over the 'WalletClient'.
hoistClient :: (forall x. m x -> n x) -> WalletClient m -> WalletClient n
hoistClient phi =
    natMapClient phi id

-- | Generalize a @'WalletClient' 'IO'@ into a @('MonadIO' m) =>
-- 'WalletClient' m@.
liftClient :: MonadIO m => WalletClient IO -> WalletClient m
liftClient = hoistClient liftIO

mapClientErrors :: forall m. Monad m => ResponseErrorHandler m -> WalletClient m -> WalletClient m
mapClientErrors handler =
    natMapClient id overError

KtorZ · 2018-09-08T10:04:16Z

@disassembler @cleverca22
Just notifying you guys about this configuration change:

https://github.com/input-output-hk/cardano-sl/pull/3431/files#diff-61feea0f7a7bc722b74eedc3907be256

This can most probably be left as null for now, unless an exchange explicitly requires it. Not sure how this impacts the installer or surrounding scripts.

angerman · 2018-09-10T07:44:53Z

🙄 👀

akegalj

Looks good to me.

I am a bit worried that the defaults are not right compared to V0 (althought its hard to tell because we didn't benchmark it)

akegalj · 2018-09-10T10:30:46Z

  background process.

+- #### Add bouncing and throttling to the API
+  Previously, exchanges could accidentally overload their wallet servers. We


there was a throttling in v0 per handler https://github.com/input-output-hk/cardano-sl/blob/develop/wallet/src/Pos/Wallet/Web/Methods/Payment.hs#L80

akegalj · 2018-09-10T10:36:27Z

+    # # Number of requests per throttling period.
+    #   rate: 200
+    #   # Amount of microseconds in a throttling period.
+    #   period: 1_000_000


https://github.com/input-output-hk/cardano-sl/blob/develop/wallet/src/Pos/Wallet/Web/Methods/Payment.hs#L80 note that we have "throttled" 6 seconds here - which is 6 times more then here. I know new data layer is probably more optimized then the old one - but maybe we should measure or check why did we pick 6 in v0. I know it was due to sev1 performance problem

Note that this is actually quite a bit faster -- we have rate requests per period microseconds, so 200 requests permitted per second in this configuration, across the board. It is disabled entirely by default.

More fine-grained throttling can easily be done at the handler level, I think.

so 200 requests permitted per second in this configuration

yes - we just have to check eventually is our wallet layer up to the task. I am not sure have we run benchmarks on new wallet layer and compared it to the old one

akegalj · 2018-09-10T10:41:24Z

+defaultThrottleSettings :: ThrottleSettings
+defaultThrottleSettings = ThrottleSettings
+    { tsRate = 30
+    , tsPeriod = 1000000


maybe set to 6sec? see https://github.com/input-output-hk/cardano-sl/blob/develop/wallet/src/Pos/Wallet/Web/Methods/Payment.hs#L80

akegalj · 2018-09-13T20:13:41Z

@parsonsmatt
note that I wanted to do few modification to your PR before it gets merged to develop #3581

parsonsmatt · 2018-09-13T22:22:11Z

@akegalj I merged because there are configuration changes that were nightmarish to deal with along with other work that's going on in the wallet. Target that PR against develop and it should be fine.

parsonsmatt force-pushed the Squad1/CBR-179/bouncing-and-throttling branch from e300523 to ae68910 Compare August 17, 2018 20:09

cleverca22 force-pushed the Squad1/CBR-179/bouncing-and-throttling branch from a59ebf8 to 23aec43 Compare September 6, 2018 13:38

parsonsmatt requested review from KtorZ and adinapoli-iohk September 7, 2018 20:05

KtorZ approved these changes Sep 8, 2018

View reviewed changes

parsonsmatt added 3 commits September 8, 2018 11:50

[CO-349] Implement throttling middleware

4f9bc72

- Format message as JSend, include retry information - Use the API error mechanism so it can be caught more easily on client side - Add to sample, test

[CO-349] Add the mapClientErrors function

3462791

This function allows you to uniformly add an error handler to each endpoint of the WalletClient type. This will be used in an upcoming commit to provide the throttling retry logic.

[CO-349] Implement withThrottlingRetry and use in integration tests

7065576

This should prevent the failures from throttling to occur in integration testing until we can have file-level configuration of the integration test.

KtorZ force-pushed the Squad1/CBR-179/bouncing-and-throttling branch from 9b848b6 to a4b14e8 Compare September 8, 2018 09:56

parsonsmatt added 2 commits September 8, 2018 14:14

[CO-350] Pass the wallet configuration around

62defe4

[CBR-179] Add changelog entry

680c933

KtorZ force-pushed the Squad1/CBR-179/bouncing-and-throttling branch from 70665d7 to 3d2e22b Compare September 8, 2018 12:14

[CBR-179] Update pkgs/default.nix

549e5be

KtorZ force-pushed the Squad1/CBR-179/bouncing-and-throttling branch from 3d2e22b to 549e5be Compare September 10, 2018 06:30

akegalj approved these changes Sep 10, 2018

View reviewed changes

Merge branch 'develop' into Squad1/CBR-179/bouncing-and-throttling

7d541a4

angerman force-pushed the Squad1/CBR-179/bouncing-and-throttling branch 4 times, most recently from 06d8c0f to 7d541a4 Compare September 12, 2018 07:01

akegalj mentioned this pull request Sep 12, 2018

[CBR-179] Add requst throttling for new wallet layer #3581

Merged

12 tasks

merge develop

0581de1

parsonsmatt merged commit 267099f into develop Sep 13, 2018

Conversation

parsonsmatt commented Aug 17, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

Uh oh!

KtorZ left a comment

Choose a reason for hiding this comment

Uh oh!

KtorZ commented Sep 8, 2018

Uh oh!

KtorZ commented Sep 8, 2018

Uh oh!

KtorZ commented Sep 8, 2018

Uh oh!

angerman commented Sep 10, 2018

Uh oh!

akegalj left a comment

Choose a reason for hiding this comment

Uh oh!

akegalj Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

akegalj Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

akegalj Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

akegalj Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

akegalj commented Sep 13, 2018

Uh oh!

parsonsmatt commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants