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

Logging Middleware: Add GitHub API Rate Limiting information #413

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions githubapp/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ func ClientMetrics(registry metrics.Registry) ClientMiddleware {
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))
updateRegistryForHeader(res.Header, HTTPHeaderRateLimit, metrics.GetOrRegisterGauge(limitMetric, registry))
updateRegistryForHeader(res.Header, HTTPHeaderRateRemaining, metrics.GetOrRegisterGauge(remainingMetric, registry))
// TODO Think about to add X-Ratelimit-Used, X-Ratelimit-Reset and X-Ratelimit-Resource as well
}

return res, err
Expand Down
53 changes: 53 additions & 0 deletions githubapp/middleware_logging.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,21 @@ import (
"io"
"net/http"
"regexp"
"strconv"
"time"

"github.com/gregjones/httpcache"
"github.com/rs/zerolog"
)

const (
HTTPHeaderRateLimit = "X-Ratelimit-Limit"
HTTPHeaderRateRemaining = "X-Ratelimit-Remaining"
HTTPHeaderRateUsed = "X-Ratelimit-Used"
HTTPHeaderRateReset = "X-Ratelimit-Reset"
HTTPHeaderRateResource = "X-Ratelimit-Resource"
)

// ClientLogging creates client middleware that logs request and response
// information at the given level. If the request fails without creating a
// response, it is logged with a status code of -1. The middleware uses a
Expand Down Expand Up @@ -83,6 +92,7 @@ func ClientLogging(lvl zerolog.Level, opts ...ClientLoggingOption) ClientMiddlew
Int64("size", -1)
}

addRateLimitInformationToLog(options.LogRateLimitInformation, evt, res)
evt.Msg("github_request")
return res, err
})
Expand All @@ -95,6 +105,18 @@ type ClientLoggingOption func(*clientLoggingOptions)
type clientLoggingOptions struct {
RequestBodyPatterns []*regexp.Regexp
ResponseBodyPatterns []*regexp.Regexp

// Output control
LogRateLimitInformation *RateLimitLoggingOption
}

// RateLimitLoggingOption controls which rate limit information is logged.
type RateLimitLoggingOption struct {
Limit bool
Remaining bool
Used bool
Reset bool
Resource bool
}

// LogRequestBody enables request body logging for requests to paths matching
Expand All @@ -117,6 +139,14 @@ func LogResponseBody(patterns ...string) ClientLoggingOption {
}
}

// SetRateLimitInformation defines which rate limit information like
// the number of requests remaining in the current rate limit window is getting logged.
func SetRateLimitInformation(options *RateLimitLoggingOption) ClientLoggingOption {
return func(opts *clientLoggingOptions) {
opts.LogRateLimitInformation = options
}
}

func mirrorRequestBody(r *http.Request) (*http.Request, []byte, error) {
switch {
case r.Body == nil || r.Body == http.NoBody:
Expand Down Expand Up @@ -174,3 +204,26 @@ func requestMatches(r *http.Request, pats []*regexp.Regexp) bool {
func closeBody(b io.ReadCloser) {
_ = b.Close() // per http.Transport impl, ignoring close errors is fine
}

func addRateLimitInformationToLog(loggingOptions *RateLimitLoggingOption, evt *zerolog.Event, res *http.Response) {
if limitHeader := res.Header.Get(HTTPHeaderRateLimit); loggingOptions.Limit && limitHeader != "" {
limit, _ := strconv.Atoi(limitHeader)
evt.Int("ratelimit-limit", limit)
}
if remainingHeader := res.Header.Get(HTTPHeaderRateRemaining); loggingOptions.Remaining && remainingHeader != "" {
remaining, _ := strconv.Atoi(remainingHeader)
evt.Int("ratelimit-remaining", remaining)
}
if usedHeader := res.Header.Get(HTTPHeaderRateUsed); loggingOptions.Used && usedHeader != "" {
used, _ := strconv.Atoi(usedHeader)
evt.Int("ratelimit-used", used)
}
if resetHeader := res.Header.Get(HTTPHeaderRateReset); loggingOptions.Reset && resetHeader != "" {
if v, _ := strconv.ParseInt(resetHeader, 10, 64); v != 0 {
evt.Time("ratelimit-reset", time.Unix(v, 0))
}
}
if resourceHeader := res.Header.Get(HTTPHeaderRateResource); loggingOptions.Resource && resourceHeader != "" {
evt.Str("ratelimit-resource", resourceHeader)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.