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

Request: Better errors handling and a request limiter #4

Closed
FrostBy opened this issue Apr 22, 2018 · 9 comments
Closed

Request: Better errors handling and a request limiter #4

FrostBy opened this issue Apr 22, 2018 · 9 comments

Comments

@FrostBy
Copy link

FrostBy commented Apr 22, 2018

Hi!
I guess we need a bit better errors handler at the line 59
For example, OD can throws an exception related to the requests limit per second and it's a bit tricky to a new user to understand what happened, because the error value will be NULL. So, please, throw the reject with an object :

reject({
    'err':            err,
    "response": response,
    "body":       body
});

or smth like this.

@7596ff
Copy link
Owner

7596ff commented Apr 22, 2018

Okay sure thing.

@7596ff
Copy link
Owner

7596ff commented Apr 22, 2018

Should be fixed in e843406, but the bucket code should handle the ratelimit anyway by only sending one per second. However I really should update that to respect 60/m instead of 1/s.

@7596ff 7596ff closed this as completed Apr 22, 2018
@FrostBy
Copy link
Author

FrostBy commented Apr 22, 2018

Also, it will be great to add to the module any limiter.
I use Bottleneck
For example:

const limiter = new Bottleneck({
    maxConcurrent: 1,
    minTime:       1000,
    strategy:      Bottleneck.strategy.BLOCK, // see the comment bellow
    reservoir:     null // see the comment bellow
});

// I use these params to block a queue for 10 sec when OD throws me exceptions and to return any rejected item to the queue.

let getTopPlayers = function (data) {
    limiter.updateSettings({reservoir: null});

    Object.keys(data).forEach(function (key) {
        let hero = heroes[key];
        limiter.schedule(hero => mika.getRankings(hero.id), hero)
            .then((players) => {
                 //do whatever you want
            })
            .catch(err => {
                limiter.updateSettings({reservoir: 0});
                sleep(10000); // system-sleep module to avoid unnesessary calls to the API. 2-3 calls and OD will allow to retrieve the data again.
                let new_data = {
                    [key]: hero
                };
                getTopPlayers(new_data); //add the item to the queue again
            });
    });
};

@7596ff
Copy link
Owner

7596ff commented Apr 22, 2018

Oh this is neat, I'll have a look at it and implement it when I get a chance

@FrostBy FrostBy changed the title Request: Better errors handling Request: Better errors handling and a request limiter Apr 22, 2018
@7596ff
Copy link
Owner

7596ff commented Apr 22, 2018

Version 1.5.0 has been published using the Bottleneck library instead of the old Bucket code. However it still requests 1/s (or 1/0.2s for key users) because I can't figure out how to get it to request as soon as possible without dropping jobs. The strategies are hard to understand, but as I read them, it either will fail to schedule or submit jobs if it is in blocking mode which is a little weird.

@FrostBy
Copy link
Author

FrostBy commented Apr 22, 2018

Let me explain how to avoid any errors.
I will provide some code snippets.

  1. We need to initialize Bottleneck with the params which I provided previously
let limiter = new Bottleneck({
    maxConcurrent: 1,
    minTime:       1000,
    strategy:      Bottleneck.strategy.BLOCK,
    reservoir:     null
});

It means that we will have the only one worker, that will init the jobs with 1 sec timeout.
Reservoir- this is the limit of jobs. Bottleneck decrements it's value after every job.
Strategy - it explain how Bottleneck should react on new jobs. Block means that Bottleneck will ignore any incoming jobs.
2) So..

let getTopPlayers = function (data) {
...
 limiter.updateSettings({reservoir: null});
...
 limiter.schedule(hero => mika.getRankings(hero.id), hero)
            .then((players) => {
                 //do whatever you want
            })
            .catch(err => {
                limiter.updateSettings({reservoir: 0});
                sleep.mspleep(10000);
                let new_data = {
                    [key]: hero
                };
                getTopPlayers(new_data); //add the item to the queue again
            });
...
}

When a job fails, I block the job using reservoir, make my app sleep for 10 sec(avoid unnesessary calls to API when it blocked due to the request limit) and append the job to the end for the chain of jobs

Here the simplest example for parsing matches by IDs

const sleep = require("sleep");
const mika = new Mika();
const fs = require('fs');
const Bottleneck = require("bottleneck");

let limiter = new Bottleneck({
    maxConcurrent: 1,
    minTime:       1000,
    strategy:      Bottleneck.strategy.BLOCK,
    reservoir:     null
});

let downloadMatches = function () {
    fs.readFile('./matches.json', function (err, data) { // array of matches [id,id,id,...]
        _getMatchesData(JSON.parse(data));
    });
};

let _getMatchesData = function (matches) {
    limiter.updateSettings({reservoir: null});

    matches.forEach(function (match, index) {
        limiter.schedule((id) => mika.getMatch(id), match)
            .then((match) => {
                fs.writeFile('./matches/' + match.match_id + '.json', JSON.stringify(match), (err) => {
                    if (err) throw err;
                });
            })
            .catch(err => {
                limiter.updateSettings({reservoir: 0});
                sleep.msleep(10000);
                let matches_new = [match];
                _getMatchesData(matches_new);
            });
    });
};

default
default

@FrostBy
Copy link
Author

FrostBy commented Apr 22, 2018

I just need to add a logic to filter the 50.000 limit of request per day and catch other uncommon cases

@7596ff
Copy link
Owner

7596ff commented Apr 22, 2018

I'm reading this blocked strategy documentation:

Bottleneck.strategy.BLOCK
When adding a new job to a limiter, if the queue length reaches highWater, the limiter falls into "blocked mode". All queued jobs are dropped and no new jobs will be accepted until the limiter unblocks. It will unblock after penalty milliseconds have passed without receiving a new job. penalty is equal to 15 * minTime (or 5000 if minTime is 0) by default and can be changed by calling changePenalty(). This strategy is ideal when bruteforce attacks are to be expected. This strategy totally ignores priority levels.

To me this seems like if a user makes too many requests the internal Bottleneck object will drop all the queued calls. So either the user needs access to the internal Bottleneck object or there needs to be library logic to auto retry on a request. I'm thinking that the way I initialize the Bottleneck object right now will just simply only make requests every so often, and the user should never hit a rate limit unless they run out of calls for the month.

Now as for your case couldn't you just intercept the error that the library gives you and retry it again using the internal queue?

@FrostBy
Copy link
Author

FrostBy commented Apr 22, 2018

Opendota throws the error even with minTime: 1000, I guess OD has bugs or my channel bandwidth makes some latencies.
I'll try to use v1.5.0 a bit later. I need ot parse more than 300.000 matches ASAP so after this i'll try to test the new version :)

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

No branches or pull requests

2 participants