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

log 429s to sentry (attempt 2); affects [dynamic endpoint uptimerobot weblate opencollective discord github] #9546

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Sep 4, 2023

Refs #9505
Refs #9523
Closes #9524

@chris48s chris48s added service-badge New or updated service badge developer-experience Dev tooling, test framework, and CI core Server, BaseService, GitHub auth, Shared helpers labels Sep 4, 2023
@@ -74,6 +74,7 @@ export default class UptimeRobotBase extends BaseJsonService {
...opts,
},
},
logErrors: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

See #9155 for details.

The rate limits we're hitting here are on the user-supplied tokens. There's not really anything we can do about this.

@@ -16,7 +16,7 @@ export default class OpencollectiveAll extends OpencollectiveBase {
},
}

static _cacheLength = 900
static _cacheLength = 1800
Copy link
Member Author

Choose a reason for hiding this comment

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

Lets see if this gets us consistently under the limits. If not, I'll raise an issue with OC and see if they'd be willing to grant us a higher limit.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

Warnings
⚠️ This PR modified service code for discord but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for dynamic but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for github but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for opencollective but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for weblate but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS against 1b0e7d3

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Changes look reasonable to me. Best to filter out errors we can't act on, otherwise it's just noise. Hopefully cache increase on the others will do the job.

@chris48s chris48s added this pull request to the merge queue Dec 4, 2023
Merged via the queue into badges:master with commit 45bb786 Dec 4, 2023
20 checks passed
@chris48s chris48s deleted the 429s-part2 branch December 4, 2023 13:43
Lordfirespeed pushed a commit to Lordfirespeed/shields that referenced this pull request Dec 18, 2023
… weblate opencollective discord github] (badges#9546)

* log to sentry if upstream service responds with 429

* allow services to decide which error(s) to log, default to 429

* don't log 429s from endpoint or dynamic badges

* supress 429s from uptime robot badges

* supress 429s from weblate if not calling default server

* cache opencollective badges for longer

* cache discord badges for longer

* cache github workflow badges for longer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers developer-experience Dev tooling, test framework, and CI service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Triage 429s in Sentry
2 participants