Skip to content

Logging Middleware: Add GitHub API Rate Limiting information#413

Merged
bluekeyes merged 9 commits intopalantir:developfrom
andygrunwald:rate-limit-info-in-logging-middleware
Feb 4, 2025
Merged

Logging Middleware: Add GitHub API Rate Limiting information#413
bluekeyes merged 9 commits intopalantir:developfrom
andygrunwald:rate-limit-info-in-logging-middleware

Conversation

@andygrunwald
Copy link
Copy Markdown
Contributor

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.

Comment thread githubapp/middleware_logging.go
@andygrunwald
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

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 andygrunwald requested a review from asvoboda February 1, 2025 13:51
@andygrunwald
Copy link
Copy Markdown
Contributor Author

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

@asvoboda
Copy link
Copy Markdown
Member

asvoboda commented Feb 3, 2025

Sorry about that. Can you also update the go.mod dependency on latest so the build will pass? However, I'm not sure if we want to take a non tagged dependency.

@bluekeyes
Copy link
Copy Markdown
Contributor

The build is failing due to a panic in the tests, not due to dependency issues. As mentioned in this comment, we don't actually depend on any new functionality in google/go-github for this.

--- FAIL: TestClientLogging (0.00s)
    --- FAIL: TestClientLogging/requestBody (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x6043be]

goroutine 29 [running]:
testing.tRunner.func1.2({0x649a[20](https://github.com/palantir/go-githubapp/actions/runs/12990574161/job/36605077242?pr=413#step:7:21), 0x8a4bb0})
	/opt/hostedtoolcache/go/1.23.5/x64/src/testing/testing.go:1632 +0x230
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.23.5/x64/src/testing/testing.go:1635 +0x35e
panic({0x649a20?, 0x8a4bb0?})
	/opt/hostedtoolcache/go/1.[23](https://github.com/palantir/go-githubapp/actions/runs/12990574161/job/36605077242?pr=413#step:7:24).5/x64/src/runtime/panic.go:785 +0x132
github.com/palantir/go-githubapp/githubapp.addRateLimitInformationToLog(0x0, 0xc0001[24](https://github.com/palantir/go-githubapp/actions/runs/12990574161/job/36605077242?pr=413#step:7:25)5b0, 0xc000154e10)
	/home/runner/work/go-githubapp/go-githubapp/githubapp/middleware_logging.go:209 +0x3e
github.com/palantir/go-githubapp/githubapp.ClientLogging.func1.1(0xc00017ba40)
	/home/runner/work/go-githubapp/go-githubapp/githubapp/middleware_logging.go:95 +0x418

Comment thread githubapp/middleware_logging.go Outdated
Comment thread githubapp/middleware_logging.go Outdated
Comment thread githubapp/middleware_logging.go Outdated
@andygrunwald
Copy link
Copy Markdown
Contributor Author

@asvoboda This PR doesn't require a go-github update, as we don't depend on it. This was a false information from my initially. Sorry for the confusion.
Additionally, a new go-github release hasn't been created yet, which includes google/go-github#3453

@bluekeyes Thanks for the additional review. I made the requested changes. The unit tests should also work now (at least with go test ./... they are passing on my machine).

The new logging output (using a zerolog.Dict()) looks now like:

With full rate limit information

2025-02-04T08:40:57+01:00 INF github_request cached=false elapsed=675.925833 method=GET path=https://api.github.com/installation/repositories?per_page=100 ratelimit={"limit":5000,"remaining":4999,"reset":"2025-02-04T09:40:57+01:00","resource":"core","used":1} size=-1 status=200

Without used and resource

2025-02-04T08:42:12+01:00 INF github_request cached=false elapsed=594.985792 method=GET path=https://api.github.com/installation/repositories?per_page=100 ratelimit={"limit":5000,"remaining":4998,"reset":"2025-02-04T09:40:57+01:00"} size=-1 status=200

Without rate limit logging information at all

2025-02-04T08:43:26+01:00 INF github_request cached=false elapsed=573.499125 method=GET path=https://api.github.com/installation/repositories?per_page=100 size=-1 status=200

Copy link
Copy Markdown
Contributor

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the contribution and for iterating with us on the details!

@bluekeyes bluekeyes merged commit 2c9fcbf into palantir:develop Feb 4, 2025
@andygrunwald
Copy link
Copy Markdown
Contributor Author

@bluekeyes My pleasure. Thanks for the help.
Would be great to see a new release with this :)

@andygrunwald
Copy link
Copy Markdown
Contributor Author

Oh, its out already: https://github.com/palantir/go-githubapp/releases/tag/v0.32.1
You are fast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants