Skip to content

feat: Add GitHub API rate limit metrics #22864

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 42 commits into from
Jun 17, 2025
Merged

feat: Add GitHub API rate limit metrics #22864

merged 42 commits into from
Jun 17, 2025

Conversation

reggie-k
Copy link
Member

@reggie-k reggie-k commented May 4, 2025 •
edited
Loading

Closes #22961

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

reggie-k added 5 commits April 30, 2025 08:44
@reggie-k
metrics
779071b 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
e956535 
Merge branch 'master' of https://github.com/argoproj/argo-cd into add-github-api-rate-limit-metrics
@reggie-k
refactor tests
3a67e56 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
add logging
626b9d3 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
8eb6995 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented May 4, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@reggie-k
master sync
a214b93 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@codecov Codecov
Copy link

codecov bot commented May 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.05628% with 53 lines in your changes missing coverage. Please review.

Project coverage is 60.15%. Comparing base (2801a11) to head (1112237).
Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
applicationset/generators/scm_provider.go 41.66% 12 Missing and 2 partials ⚠️
...licationset/services/internal/github_app/client.go 0.00% 14 Missing ⚠️
applicationset/generators/pull_request.go 18.75% 11 Missing and 2 partials ⚠️
applicationset/services/pull_request/github_app.go 0.00% 3 Missing ⚠️
applicationset/services/scm_provider/github_app.go 0.00% 3 Missing ⚠️
applicationset/utils/utils.go 40.00% 2 Missing and 1 partial ⚠️
cmd/argocd-server/commands/argocd_server.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #22864      +/-   ##
==========================================
+ Coverage   60.07%   60.15%   +0.08%     
==========================================
  Files         342      343       +1     
  Lines       58814    59003     +189     
==========================================
+ Hits        35330    35496     +166     
- Misses      20623    20648      +25     
+ Partials     2861     2859       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

reggie-k added 8 commits May 4, 2025 18:49
@reggie-k
master sync
af00611 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
master sync
58618c7 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
fb3b201 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
added docs
d1ee096 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
added docs
14c9ad4 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
added docs
ba678ca 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
better tests and logging
52d0dc0 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
better tests and logging
8d632c6 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k reggie-k changed the title feat: Add GitHub api rate limit metrics feat: Add GitHub API rate limit metrics May 8, 2025
@reggie-k reggie-k marked this pull request as ready for review May 11, 2025 07:16
@reggie-k reggie-k requested review from a team as code owners May 11, 2025 07:16
@pasha-codefresh
Copy link
Member

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented May 18, 2025 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit 1112237)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

High Cardinality

Using the full request path as a metric label (endpoint := req.URL.Path) may lead to high cardinality if paths include dynamic segments or query parameters.

 
endpoint := req.URL.Path
method := req.Method
Registry Conflicts

Global metrics are registered unconditionally in init, which could cause duplicate registration errors or leaking metrics across different registries or tests.

 
func init() {
	log.Debug("Registering GitHub API AppSet metrics")
	metrics.Registry.MustRegister(globalGitHubMetrics.RequestTotal)
	metrics.Registry.MustRegister(globalGitHubMetrics.RequestDuration)
	metrics.Registry.MustRegister(globalGitHubMetrics.RateLimitRemaining)
	metrics.Registry.MustRegister(globalGitHubMetrics.RateLimitLimit)
	metrics.Registry.MustRegister(globalGitHubMetrics.RateLimitReset)
	metrics.Registry.MustRegister(globalGitHubMetrics.RateLimitUsed)
HTTP Client Default

GetOptionalHTTPClient returns a new http.Client{} with no timeout or transport settings, diverging from http.DefaultClient behavior.

 
func GetOptionalHTTPClient(optionalHTTPClient ...*http.Client) *http.Client {
	if len(optionalHTTPClient) > 0 && optionalHTTPClient[0] != nil {
		return optionalHTTPClient[0]
	}
	return &http.Client{}
}

@pasha-codefresh
Copy link
Member

@reggie-k i think as follow up PR it would be nice to provide dashboard that will show usage of these metrics

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit e097974

pasha-codefresh
pasha-codefresh previously requested changes May 18, 2025
View reviewed changes
reggie-k added 2 commits May 25, 2025 14:57
@reggie-k
upstream sync
8c30133 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
dependent on cmd-params config and optional with default transport
ccdd232 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
reggie-k and others added 7 commits May 29, 2025 15:30
@reggie-k
keep transport with auth
4e1fca8 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
c686fe1 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
better handling of custom transport
ccc2b45 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
784929d 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
placate linter
e3b71a0 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
e5b0aab 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
Merge branch 'master' into add-github-api-rate-limit-metrics
1dd84ce
crenshaw-dev
crenshaw-dev reviewed Jun 13, 2025
View reviewed changes
crenshaw-dev
crenshaw-dev reviewed Jun 13, 2025
View reviewed changes
agaudreault
agaudreault requested changes Jun 13, 2025
View reviewed changes
reggie-k and others added 7 commits June 15, 2025 15:46
@reggie-k
Merge branch 'master' of https://github.com/argoproj/argo-cd into add…
8d37827 
…-github-api-rate-limit-metrics

Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k @agaudreault
Update applicationset/services/github_metrics.go
e68657f 
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: Regina Voloshin <regina.voloshin@codefresh.io>
@reggie-k
resource as a label instead of a metric
8953aa0 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k @agaudreault
Update applicationset/services/github_metrics.go
f714b0d 
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: Regina Voloshin <regina.voloshin@codefresh.io>
@reggie-k
resolved review comments
d79c7fc 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
resolved review comments
4d55d69 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
resolved review comments
3146c02 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@agaudreault agaudreault added this to the v3.1 milestone Jun 16, 2025
agaudreault
agaudreault requested changes Jun 16, 2025
View reviewed changes
Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM. Small nitpick

reggie-k added 2 commits June 17, 2025 03:08
@reggie-k
removed creds type
35a5ea7 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@reggie-k
metrics.md clarifications
1112237 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
@agaudreault agaudreault enabled auto-merge (squash) June 17, 2025 01:13
@agaudreault agaudreault disabled auto-merge June 17, 2025 01:13
@agaudreault agaudreault enabled auto-merge (squash) June 17, 2025 01:13
@agaudreault agaudreault dismissed pasha-codefresh’s stale review June 17, 2025 01:17

Comments were addressed

@agaudreault agaudreault merged commit 964f269 into argoproj:master Jun 17, 2025
26 of 27 checks passed
@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 1112237

@voidlily voidlily mentioned this pull request Jun 30, 2025
Closed
enneitex pushed a commit to enneitex/argo-cd that referenced this pull request Aug 24, 2025
@reggie-k @agaudreault @enneitex
feat: Add GitHub API rate limit metrics (argoproj#22864)
60a6d44 
Signed-off-by: reggie-k <regina.voloshin@codefresh.io>
Signed-off-by: Regina Voloshin <regina.voloshin@codefresh.io>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: enneitex <etienne.divet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crenshaw-dev crenshaw-dev crenshaw-dev left review comments

@agaudreault agaudreault agaudreault approved these changes

@pasha-codefresh pasha-codefresh pasha-codefresh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1
Development

Successfully merging this pull request may close these issues.

feat: add GitHub API invocation and rate limit metrics
5 participants
@reggie-k @pasha-codefresh @CodiumAI-Agent @crenshaw-dev @agaudreault