go-github icon indicating copy to clipboard operation
go-github copied to clipboard

A callback to go with SleepUntilPrimaryRateLimitResetWhenRateLimited?

Open dee-kryvenko opened this issue 1 year ago • 7 comments

A library this repository suggests for secondary rate limits https://github.com/gofri/go-github-ratelimit already has a similar concept WithLimitDetectedCallback(callback). There is no easy way right now to handle sleeps i.e. log a message to the terminal or something...

dee-kryvenko avatar Jul 26 '24 17:07 dee-kryvenko

Sleeps should be handled in your client code that uses this library.

We have an entire section in our README.md devoted to the topic: https://github.com/google/go-github/tree/master?tab=readme-ov-file#rate-limiting

I hope that helps.

gmlewis avatar Jul 26 '24 18:07 gmlewis

@gmlewis I am sorry, this response is confusing. This repository already have a sleep logic, and it is explained in the README.md right below the place you linked:

repos, _, err := client.Repositories.List(context.WithValue(ctx, github.SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "", nil)

What I am talking about, is a callback next to the sleep, because right now the information that the sleep did occur is not exposed outside so my program would just freeze without an opportunity to me notify a user or log a message.

I am not sharing the thought that the client code should handle this. I can as well just use http client instead of this library. And ASM instead of Go. How deep should we go, really? How is the amount of code the client needs to do to check for the rate limit and semaphore in a multi threaded environment can be justified comparatively to three lines it takes to invoke a callback right around here https://github.com/google/go-github/blob/020ef00cd841733048f836554efa4347a886bb72/github/github.go#L894?

dee-kryvenko avatar Jul 26 '24 20:07 dee-kryvenko

Sorry I misunderstood. Reopening.

gmlewis avatar Jul 26 '24 21:07 gmlewis

@erezrokah - can you please comment on this issue since you added this feature in #3117 ?

gmlewis avatar Jul 26 '24 21:07 gmlewis

Hi @dee-kryvenko and @gmlewis the feature request makes sense to me, it's even listed in the possible future improvements in the original PR https://github.com/google/go-github/pull/3117.

If anyone has a suggestion on how to configure the callback, happy to comment/review it on it

erezrokah avatar Jul 29 '24 08:07 erezrokah

Well, if we were to keep the precedent that is already established, it sounds like we need a new context key with function as a value. I think function should have a simple no args no returns signature and upstream code can handle everything asynchronous via channels. So this needs to just check if the context is set, cast the value, and call it. Question is - do we want this to be at the level of checking existing context key, which I can see two places there, or do we want to put that directly in the sleep function?

dee-kryvenko avatar Jul 29 '24 15:07 dee-kryvenko

I would follow a similar behavior as in https://github.com/gofri/go-github-ratelimit?tab=readme-ov-file#client-options, where the callback accepts some information on the request that is throttled, the sleep time, etc. and is invoked just before the sleep happens

erezrokah avatar Jul 30 '24 12:07 erezrokah