gnikyt / Basic-Shopify-API

A simple API wrapper for Shopify using Guzzle for REST and GraphQL
MIT License
220 stars 66 forks source link

API Rate Limiting #52

Closed DaveTacker closed 4 years ago

DaveTacker commented 4 years ago

I'm finding that whatever settings I enter into $this->api->enableRateLimiting(750, 500); doesn't seem to be working, or I'm misunderstanding something. To test it, I've created a function:

public function testDelay() {
    for ($i = 1; $i <= 5; $i++) {
        $job = new DelayTest($i, new DateTime('now'));
        $this->dispatch($job);
    }
    echo "\n";
}

The job being executed reaches out to Shopify and pulls an index on orders from another controller. Here's the Job:

class DelayTest implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    private $iteration;
    private $then;

    /**
     * Create a new job instance.
     *
     * @return void
     */
    public function __construct(int $iteration, DateTime $then)
    {
        $this->iteration = $iteration;
        $this->then = $then;
    }

    /**
     * Execute the job.
     *
     * @return void
     */
    public function handle()
    {
        $now = new DateTime('now');
        $getCtrl = new GetController();
        $orders = $getCtrl->orders('1', (string)$this->iteration, '10', 'any', 'any', 'any');
        $orders = \json_decode($orders);
        echo $this->iteration . '| order id: ' . $orders[0]->id . " then: " . $this->then->format('H:i:s:v') . ", now: " . $now->format('H:i:s:v') . "\n";
    }
}

The output of the above gives me this:

1| order id: 1874921488420 then: 06:51:16:114, now: 06:51:16:118
2| order id: 1874915131428 then: 06:51:16:652, now: 06:51:16:653
3| order id: 1874907103268 then: 06:51:17:121, now: 06:51:17:122
4| order id: 1874894946340 then: 06:51:17:642, now: 06:51:17:643
5| order id: 1874877317156 then: 06:51:18:095, now: 06:51:18:096

What confuses me is that the now: timestamp isn't being incremented by 750ms. Am I approaching this correctly?

gnikyt commented 4 years ago

@kneeki The rate limiting is not for (your code), its the rate limiting support for Shopify API calls.

The package implements a very simple version of this, you're allowed to hit Shopify's API twice in one second for regular shops, or four times in one second for Plus shops.

Whats the limiter does, is it checks the header returned by Shopify. The header contains how many calls you used and have left. If you run out of calls, the library will sleep until it can gain calls back.

Does that help?

DaveTacker commented 4 years ago

So, theoretically even this basic version of a rate limiter should prevent the 429 error, right? Or does it not queue up commands and just sleeps the one request?

On Sun, Dec 1, 2019, 2:36 PM Tyler King notifications@github.com wrote:

@kneeki https://github.com/kneeki The rate limiting is not for (your code), its the rate limiting support for Shopify API calls.

The package implements a very simple version of this, you're allowed to hit Shopify's API twice in one second for regular shops, or four times in one second for Plus shops.

Whats the limiter does, is it checks the header returned by Shopify. The header contains how many calls you used and have left. If you run out of calls, the library will sleep until it can gain calls back.

Does that help?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ohmybrew/Basic-Shopify-API/issues/52?email_source=notifications&email_token=AAHXDSMUCA2E5JBIQNQW2BTQWQ37JA5CNFSM4JSVSKUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFRX34Y#issuecomment-560168435, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHXDSO736VSUVR2SHEVU6LQWQ37JANCNFSM4JSVSKUA .

gnikyt commented 4 years ago

It calculates a wait time and sleeps.

amosmos commented 4 years ago

Hey,

Two quick questions regarding the conversation in this issue:

1) You mention the limiter checks the header returned by Shopify, but in the code it seems it just keeps the timestamp of the last request it issued, and calculate the sleep time based on that, so not real check based on the Shopify header - am I missing something?

2) If I didn't miss anything, that means the limiter only "knows" about recent requests made by itself, so in the case of the OP, when the requests are made by separate jobs, the limiter will not know about the other requests - am I right or wrong? Of course if I'm wrong in question #1 and the limiter does check the header by Shopify, it will support rate limiting requests from separate jobs, so I hope I am wrong :-)

Thanks! Amos

gnikyt commented 4 years ago

The header value is used to know what's left. The timestamps are used to make sure not too many calls are happening. But yes, multiple instances would cause an issue since it's not a static value.. I would need to implement it as a static value but I believe there were issues with opcache at the time when I attempted it..

Either way it's meant to be basic, someone can implement their own if they really need to, I'll of course improve on it when I can.


From: Amos Shacham notifications@github.com Sent: Monday, May 4, 2020 9:01:00 PM To: osiset/Basic-Shopify-API Basic-Shopify-API@noreply.github.com Cc: Tyler King tyler.n.king@gmail.com; State change state_change@noreply.github.com Subject: Re: [osiset/Basic-Shopify-API] API Rate Limiting (#52)

Hey,

Two quick questions regarding the conversation in this issue:

  1. You mention the limiter checks the header returned by Shopify, but in the code it seems it just keeps the timestamp of the last request it issued, and calculate the sleep time based on that, so not real check based on the Shopify header - am I missing something?

  2. If I didn't miss anything, that means the limiter only "knows" about recent requests made by itself, so in the case of the OP, when the requests are made by separate jobs, the limiter will not know about the other requests - am I right or wrong? Of course if I'm wrong in question #1https://github.com/osiset/Basic-Shopify-API/issues/1 and the limiter does check the header by Shopify, it will support rate limiting requests from separate jobs, so I hope I am wrong :-)

Thanks! Amos

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://github.com/osiset/Basic-Shopify-API/issues/52#issuecomment-623761662, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AASO4OUICGYKBPOD2HX4YO3RP5F3JANCNFSM4JSVSKUA.

amosmos commented 4 years ago

Thanks!

It makes perfect sense.

So would you recommend to check if a request had a rate limit issue and in that case use the getApiCalls function to check when run it again? I can assume this function can be the basis for an internal mechanism that will automatically run the call again until a different response is sent (I think there's a different issue for that which you said you might work on in the future).

Also, I'm not an expert on static values so correct me if I'm wrong, but I believe that won't help in case of async queue jobs calling the api.

THANKS! Amos