Logging Middleware: Add GitHub API Rate Limiting information

[andygrunwald]

Context

This Pull Request adds the GitHub API Rate Limiting information into the Logging Middleware, if enabled.
It is disabled by default, as this can blow your log volume.

For more information on GitHub Rate Limiting, see https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#checking-the-status-of-your-rate-limit

How to use it

clientCreator, err := githubapp.NewDefaultCachingClientCreator(
	config.Github,
	githubapp.WithClientUserAgent(ClientUserAgent),
	githubapp.WithClientTimeout(60*time.Second),
	githubapp.WithClientCaching(false, func() httpcache.Cache { return httpTransportCache }),
	githubapp.WithClientMiddleware(
		githubapp.ClientMetrics(metricsRegistry),
		**githubapp.ClientLogging(zerolog.InfoLevel, githubapp.EnableRateLimitInformation()),**
	),
)

Result

Log lines will look like

2025-01-25T21:55:48+01:00 INF github_request cached=false elapsed=529.904625 method=GET path=https://api.github.com/installation/repositories?per_page=100 ratelimit-limit=5000 ratelimit-remaining=4999 ratelimit-reset=2025-01-25T22:55:48+01:00 ratelimit-resource=core ratelimit-used=1 size=-1 status=200

Depends on ...

Attention! This Pull Request depends on google/go-github#3453

The fields Used and Resource are not part of go-github yet. If google/go-github#3453 gets merged, the following steps are required to move this PR forward:

1. A new google/go-github version needs to be released
2. google/go-github need to be raised inside palantir/go-githubapp
3. This PR can move forward.

Step 2 can also be added into this PR.

Missing work

• Unit tests

Feedback

I open up this PR to get feedback early on, even if the PR is not mergeable yet.

[asvoboda]

Adding this to the logs seems friendly. Should all the options be present in the logs, or should they be configurable? I can imagine used might not be that interesting since it can be calculated from limit and remaining?

We also have metrics

go-githubapp/githubapp/middleware.go

Lines 74 to 79 in b9e479f

	limitMetric := fmt.Sprintf("%s[installation:%d]", MetricsKeyRateLimit, installationID)
	remainingMetric := fmt.Sprintf("%s[installation:%d]", MetricsKeyRateLimitRemaining, installationID)
	// Headers from https://developer.github.com/v3/#rate-limiting
	updateRegistryForHeader(res.Header, "X-RateLimit-Limit", metrics.GetOrRegisterGauge(limitMetric, registry))
	updateRegistryForHeader(res.Header, "X-RateLimit-Remaining", metrics.GetOrRegisterGauge(remainingMetric, registry))

that expose similar information. IIRC the rate limits are per installation ID. I think that can be inferred based on the org?

[asvoboda]

Separately, it would be good to update the metrics headers to use the new constants you've defined here.

[andygrunwald]

Should all the options be present in the logs, or should they be configurable? I can imagine used might not be that interesting since it can be calculated from limit and remaining?

I was thinking the same. In the first try, I went with "all or nothing". I am happy to change it to be configurable for every item.

We also have metrics that expose similar information. IIRC the rate limits are per installation ID. I think that can be inferred based on the org?

Yes and no. The main rate limit depends on the org. However, there are now multiple rate limit "pools", depending on the resource (search, audit log, ...). See https://docs.github.com/en/rest/rate-limit/rate-limit?apiVersion=2022-11-28#get-rate-limit-status-for-the-authenticated-user

Quoting:

Some categories of endpoints have custom rate limits that are separate from the rate limit governing the other REST API endpoints. For this reason, the API response categorizes your rate limit.

More details about the dedicated resource areas can be found in the link.
I am not sure about what you mean with the metrics. Are you requesting that I add those items (used, resource) there as well? Would be great if you can expand on this a bit.

Separately, it would be good to update the metrics headers to use the new constants you've defined here.

Sure. I will do it.

[asvoboda]

For what its worth: I'm mostly concerned with how this reports metrics for GHES, since thats our most important consumer internally, but it seems like these are largely the same metrics according to the docs: https://docs.github.com/en/enterprise-server@3.13/rest/rate-limit/rate-limit?apiVersion=2022-11-28#get-rate-limit-status-for-the-authenticated-user

Re metrics, sorry for not being more clear. I was thinking it would be nice to expose similar information in both the metrics and the logs and let users select their most preferable approach for consuming this information. However, I was (and maybe still am) confused on how the pools are calculated compared to the existing per installation limits. I'll read up on this a bit more.

[andygrunwald]

@asvoboda I made a few changes based on your feedback:

In 25da132 and 1d48f15, i renamed the HTTP Header constant and used them also in the Metrics middleware.

In d07099e I added a note to (maybe) add those new information also into the metrics. I am suggesting to refrain from this in this PR, as it sound a bit more read up is required? It may be an idea to open up separate metrics buckets depending on the Rate limit resource (core, etc.). Just thinking about small / independent changes and not adding too much into this PR.

In afdd35b, I made every new logging information from Rate limiting configurable (through RateLimitLoggingOption).

This is changing the usage too:

clientCreator, err := githubapp.NewDefaultCachingClientCreator(
	config.Github,
	githubapp.WithClientUserAgent(ClientUserAgent),
	githubapp.WithClientTimeout(60*time.Second),
	githubapp.WithClientCaching(false, func() httpcache.Cache { return httpTransportCache }),
	githubapp.WithClientMiddleware(
		githubapp.ClientMetrics(metricsRegistry),
		githubapp.ClientLogging(zerolog.InfoLevel, githubapp.SetRateLimitInformation(&githubapp.RateLimitLoggingOption{
			Limit:     true,
			Remaining: true,
			Used:      true,
			Reset:     true,
			Resource:  true,
		})),
)

Let me know what you think.

[andygrunwald]

Side note: google/go-github#3453 is approved and is waiting for a merge. It seems that there will be a new go-github version released soon.

[andygrunwald]

Small update: google/go-github#3453 has been merged.

After a few more minutes to think about: This PR does NOT depend on a new go-github release, as we don't use variables / structures from the package in the logging middleware.

[andygrunwald]

Hey @asvoboda,
Anything i can do to push this one forward?