-
Notifications
You must be signed in to change notification settings - Fork 60
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
andygrunwald
wants to merge
5
commits into
palantir:develop
Choose a base branch
from
andygrunwald:rate-limit-info-in-logging-middleware
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b65a26f
Logging Middleware: Add GitHub API Rate Limiting information
andygrunwald 25da132
Rate Limiting Header: Rename constants from headerRate* to HTTPHeader…
andygrunwald 1d48f15
Metrics Middleware: Use new HTTPHeaderRate* constant
andygrunwald d07099e
Metrics Middleware: Add TODO about missing metrics
andygrunwald afdd35b
Logging Middleware: Make Rate limit logging options configurable
andygrunwald File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 fromlimit
andremaining
?We also have metrics
go-githubapp/githubapp/middleware.go
Lines 74 to 79 in b9e479f
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
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.
Sure. I will do it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Let me know what you think.