Go-github: rate limit exceeded: funky error message

[kevinburke]

The error message for "rate limit exceeded" has both a period and a semicolon in it which looks weird.

GET https://api.github.com/repos/golang/go/issues?direction=desc&page=1&per_page=100&sort=updated&state=all: 403 API rate limit of 5000 
still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request.; rate reset 
in 26m8.032328588s

We could also probably round the duration to the nearest second (if greater than a minute) or tenth of a second (if less than a minute) to avoid 8 useless units of precision after the decimal.

[gmlewis]

Thanks, @kevinburke... excellent points.

[dmitshur]

For anyone who wants to work on this issue, please discuss proposed fix here before sending a PR.

[dotbugfix]

Newbie here - hoping to kick off contributions with this one.

The error message originates from github/github.go:479 - removing the extra period is trivial.

I'm not sure where the "rate reset in ..." is printed, though. There's a test at github_test.go::TestDo_rateLimit_rateLimitError but it simply checks the actual members in RateLimitError. Where is this extra part concatenated to the error message?

[kevinburke]

Ah, that might have come from our error wrapper

--
Kevin Burke
925.271.7005 | kev.inburke.com

On Sat, Apr 22, 2017 at 6:25 PM, Omkar Ekbote [e-mail]
wrote:

Newbie here - hoping to kick off contributions with this one.

The error message originates from github/github.go:479 - removing the
extra period is trivial.

I'm not sure where the "rate reset in ..." is printed, though. There's a
test at github_test.go::TestDo_rateLimit_rateLimitError but it simply
checks the actual members in RateLimitError. Where is this extra part
concatenated to the error message?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/google/go-github/issues/622#issuecomment-296412808,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOSI6ZljyCVg_6JGLRxxn2u-miMJLuzks5ryqhwgaJpZM4NEb8r
.

[dmitshur]

Newbie here - hoping to kick off contributions with this one.

Thanks for looking at this!

The error message originates from github/github.go:479 - removing the extra period is trivial.

I'm not sure where the "rate reset in ..." is printed, though.

One part of the error message is indeed from github/github.go:479. The other part comes from the Error method of that RateLimitError type, at github/github.go:530-532.

So, to resolve this, we should figure out what we want the final message to be, and then decide which part of it should go into Message, and what part should be in RateLimitError.Error method.

When doing that, we should consider what a real response from GitHub API says when over rate limit. We should also consider the other error messages, such as ErrorResponse.Error, to make sure things are consistent.

[dotbugfix]

Thanks for the guidance, Dmitri.

When doing that, we should consider what a real response from GitHub API says when over rate limit.

Here's the response from the Github API (from their documentation):

HTTP/1.1 403 Forbidden
Date: Tue, 20 Aug 2013 14:50:41 GMT
Status: 403 Forbidden
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 0
X-RateLimit-Reset: 1377013266
{
   "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
   "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}

X-RateLimit-Reset is in UTC epoch seconds and Github leaves it to the application to format and display it.

We should also consider the other error messages, such as ErrorResponse.Error, to make sure things are consistent.

Here's a sample generic error message;

GET https://api.github.com/user: 401 Bad credentials []

The extra [] at the end would presumably dump the 'errors' array from an API response JSON like this:

{
  "message": "Validation Failed",
  "errors": [  //< This array would be dumped
    {
      "resource": "Issue",
      "field": "title",
      "code": "missing_field"
    }
  ]
}

[dotbugfix]

As @kevinburke suggested,

We could also probably round the duration to the nearest second (if greater than a minute) or tenth of a second (if less than a minute) to avoid 8 useless units of precision after the decimal.

.... which makes sense.

Current error message:

GET https://api.github.com/repos/golang/go/issues?direction=desc&page=1&per_page=100&sort=updated&state=all: 403 API rate limit of 5000 
still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request.; rate reset 
in 26m8.032328588s

Proposed error message(s):

a. rate reset duration <1m

GET https://api.github.com/repos/golang/go/issues?direction=desc&page=1&per_page=100&sort=updated&state=all: 403 API rate limit of 5000 
still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request; rate reset 
in 8.3s

b. rate reset duration >1m

GET https://api.github.com/repos/golang/go/issues?direction=desc&page=1&per_page=100&sort=updated&state=all: 403 API rate limit of 5000 
still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request; rate reset 
in 9m50s

[dmitshur]

Here's the response from the Github API (from their documentation):

Here's what it looks like when you get it as an error from go-github:

GET https://api.github.com/users: 403 API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.); rate reset in 58m41.516618654s

One observation is that the Message field contains a complete sentence that ends with a period. I think we should probably be consistent with that in our simulated error.

[dotbugfix]

One observation is that the Message field contains a complete sentence that ends with a period. I think we should probably be consistent with that in our simulated error.

Hmmm, I suppose adding a period at the end of the above proposed messages wouldn't hurt.

[dmitshur]

I was thinking we could keep the period at the end of not making remote request., but do something about the semicolon and onwards. For example, something like this:

... not making remote request. rate reset in 26m8s

Or like this:

... not making remote request. (rate reset in 26m8s)

[dotbugfix]

Roger that.

Proposed error message(s):

a. rate reset duration <1m

GET https://api.github.com/users: 403 API rate limit of 5000 still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request. [Rate reset in 8.3s]

b. rate reset duration >1m

GET https://api.github.com/users: 403 API rate limit of 5000 still exceeded until 2017-04-21 08:38:57 -0700 PDT, not making remote request. [Rate reset in 9m50s]

c. unauthenticated user

GET https://api.github.com/users: 403 API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.) [Rate reset in 58m41s]

I'm proposing square brackets for [Rate reset in 58m41s] since the message from Github in (c) already has a parenthesized snippet. That would make it similar to the generic error message:

GET https://api.github.com/user: 401 Bad credentials []

[dmitshur]

That looks very reasonable, I think.

About the time, I like the 58m41s format. It's compact and informative. I think we could get away displaying that even when it's less than 1 minute left, I don't think there's a point in showing sub-second precision. Instead, using a consistent "xxmyys" string would be more readable, compared to altering output depending on time left.

How about that?

• Rate reset in 58m41s
• Rate reset in 09m50s
• Rate reset in 00m08s

[dotbugfix]

If this message is intended for humans, then the leading 0s seem verbose to me. I don't think the caller would try to parse this message anyway (they'd use members in RateLimitError.Rate).

Regarding precision, I agree that being consistent is better.

• 5m54s
• 25m7s
• 19s

This is a matter of opinion, so I'm alright with swinging either way.

[dmitshur]

I'm okay with omitting leading zeros, but I think it's probably better to keep the ones in middle. E.g.:

• 5m54s
• 25m07s
• 19s

My opinion on this is not strong. That said, I'd prefer something with a simpler implementation, rather than a very complex custom one.

[dotbugfix]

Thanks, I'll start working on this and update this thread with the final fix (whichever happens to balance complexity and readability)

[dmitshur]

@korovaisdead has created PR #663 to resolve this issue, it's going through review now.

[pavelshtanko]

Just to keep this issue up to date:

It's possible for a RateLimitError to be stored for some time, and its Error method can be called after r.Rate.Reset is in the past. Then, r.Rate.Reset.Time.Sub(time.Now()) will be a negative duration, so formatDuration needs to be able to handle it.

So there are several options here:

1. [rate limit has been already reset] or something.
2. [rate limit -121m22s] this weird minus sign.
3. Do not show this block at all.

What do you guys think?

[dmitshur]

Since we're already adding custom code to control how the duration is formatted, would it be reasonable to implement something like:

[rate limit was reset 121m22s ago]

[pavelshtanko]

@shurcooL looks nice. Will do that.