From 315ca63e65a96c70d091b06a129262efa933c15a Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 24 Mar 2023 11:13:06 -0700 Subject: [PATCH 1/3] Update the scheduled event governor based upon the actions rate limit --- .../MockGitHubEventClient.cs | 12 ++ .../Constants/RateLimitConstants.cs | 21 +++ .../ScheduledEventProcessing.cs | 123 ++++++++++-------- .../GitHubEventClient.cs | 36 ++++- 4 files changed, 134 insertions(+), 58 deletions(-) create mode 100644 tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs index ecd22498043..25f560c48c1 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs @@ -455,5 +455,17 @@ public List GetGitHubIssuesToLock() { return _gitHubIssuesToLock; } + + /// + /// Override for the GitHubEventClient funcion which computes this based upon the repository's core rate limit. + /// Since this isn't necessary for the tests, just return the ActionRateLimitNonEnterprise/10. + /// + /// int +#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously + public override async Task ComputeScheduledTaskUpdateLimit() +#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously + { + return RateLimitConstants.ActionRateLimitNonEnterprise/10; + } } } diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs new file mode 100644 index 00000000000..65c08df942f --- /dev/null +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs @@ -0,0 +1,21 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Azure.Sdk.Tools.GitHubEventProcessor.Constants +{ + public class RateLimitConstants + { + // https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions + // When using the GITHUB_TOKEN the number of Actions per hour for an Enterprise repository is 15000/hour + public const int ActionRateLimitEnterprise = 15000; + // When using the GITHUB_TOKEN the number of Actions per hour for an Non-enterprise repository is 1000/hour + public const int ActionRateLimitNonEnterprise = 1000; + + // https://docs.github.com/en/rest/search?apiVersion=2022-11-28#about-search + // The SearchIssues API has a rate limit of 1000 results which resets every 60 seconds + public const int SearchIssuesRateLimit = 1000; + } +} diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs index e6bd4b0eeb8..94feb5ca96e 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs @@ -14,63 +14,68 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing { public class ScheduledEventProcessing { - // Each repository has rate limit for actions of 1000/hour. Because scheduled tasks have the potential - // to cause a large number of updates, put an initial limit on the number of updates a scheduled task - // can make. - private static int ScheduledTaskUpdateLimit = 100; - // There's a secondary limit to the search API which is 1000 items. The default page size for the search - // is 100 items (which is also the max per page) - private static int MaxSearchResults = 1000; - /// - /// + /// Scheduled events, unlike regular actions, need to have which scheduled event to run, passed in otherwise, it require + /// looking at the cron schedule to determine the event. Doing it this way means if the schedule changes only the cron + /// in the yml file needs to change rather than having to coordinate actions processing changes with the yml update. /// /// Authenticated GitHubEventClient /// ScheduledEventGitHubPayload deserialized from the json event payload - /// String, the scheduled even + /// String, the scheduled event public static async Task ProcessScheduledEvent(GitHubEventClient gitHubEventClient, ScheduledEventGitHubPayload scheduledEventPayload, string cronTaskToRun) { - switch (cronTaskToRun) + // Scheduled events can make multiple calls to SearchIssues due to pagination. Any call to SearchIssues can + // run into a SecondaryRateLimitExceededException, regardless of the page, and there could be pending updates + // from previous pages that were processed. Because of this, the call to process pending updates should be made + // regardless of whether there is an exception or not. If there aren't any updates then the call is effectively + // a no-op so it's good either way. + try { - case RulesConstants.CloseAddressedIssues: - { - await CloseAddressedIssues(gitHubEventClient, scheduledEventPayload); - break; - } - case RulesConstants.CloseStaleIssues: - { - await CloseStaleIssues(gitHubEventClient, scheduledEventPayload); - break; - } - case RulesConstants.CloseStalePullRequests: - { - await CloseStalePullRequests(gitHubEventClient, scheduledEventPayload); - break; - } - case RulesConstants.IdentifyStaleIssues: - { - await IdentifyStaleIssues(gitHubEventClient, scheduledEventPayload); - break; - } - case RulesConstants.IdentifyStalePullRequests: - { - await IdentifyStalePullRequests(gitHubEventClient, scheduledEventPayload); - break; - } - case RulesConstants.LockClosedIssues: - { - await LockClosedIssues(gitHubEventClient, scheduledEventPayload); - break; - } - default: - { - Console.WriteLine($"{cronTaskToRun} is not valid Scheduled Event rule. Please ensure the scheduled event yml is correctly passing in the correct rules constant."); - break; - } + switch (cronTaskToRun) + { + case RulesConstants.CloseAddressedIssues: + { + await CloseAddressedIssues(gitHubEventClient, scheduledEventPayload); + break; + } + case RulesConstants.CloseStaleIssues: + { + await CloseStaleIssues(gitHubEventClient, scheduledEventPayload); + break; + } + case RulesConstants.CloseStalePullRequests: + { + await CloseStalePullRequests(gitHubEventClient, scheduledEventPayload); + break; + } + case RulesConstants.IdentifyStaleIssues: + { + await IdentifyStaleIssues(gitHubEventClient, scheduledEventPayload); + break; + } + case RulesConstants.IdentifyStalePullRequests: + { + await IdentifyStalePullRequests(gitHubEventClient, scheduledEventPayload); + break; + } + case RulesConstants.LockClosedIssues: + { + await LockClosedIssues(gitHubEventClient, scheduledEventPayload); + break; + } + default: + { + Console.WriteLine($"{cronTaskToRun} is not valid Scheduled Event rule. Please ensure the scheduled event yml is correctly passing in the correct rules constant."); + break; + } + } + } + finally + { + // The second argument is IssueOrPullRequestNumber which isn't applicable to scheduled events (cron tasks) + // since they're not going to be changing a single IssueUpdate like rules processing does. + await gitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); } - // The second argument is IssueOrPullRequestNumber which isn't applicable to scheduled events (cron tasks) - // since they're not going to be changing a single IssueUpdate like rules processing does. - await gitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); } /// @@ -89,6 +94,7 @@ public static async Task CloseAddressedIssues(GitHubEventClient gitHubEventClien { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.CloseAddressedIssues)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); List includeLabels = new List { @@ -106,7 +112,7 @@ public static async Task CloseAddressedIssues(GitHubEventClient gitHubEventClien int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; for (request.Page = 1; request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); @@ -167,6 +173,8 @@ public static async Task CloseStaleIssues(GitHubEventClient gitHubEventClient, S { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.CloseStaleIssues)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); + List includeLabels = new List { LabelConstants.NeedsAuthorFeedback, @@ -185,7 +193,7 @@ public static async Task CloseStaleIssues(GitHubEventClient gitHubEventClient, S int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; for (request.Page = 1;request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); @@ -241,6 +249,7 @@ public static async Task CloseStalePullRequests(GitHubEventClient gitHubEventCli { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.CloseStalePullRequests)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); List includeLabels = new List { @@ -258,7 +267,7 @@ public static async Task CloseStalePullRequests(GitHubEventClient gitHubEventCli int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; for (request.Page = 1; request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); @@ -319,6 +328,7 @@ public static async Task IdentifyStalePullRequests(GitHubEventClient gitHubEvent { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.IdentifyStalePullRequests)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); List excludeLabels = new List { @@ -337,7 +347,8 @@ public static async Task IdentifyStalePullRequests(GitHubEventClient gitHubEvent int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; + for (request.Page = 1; request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); @@ -398,6 +409,7 @@ public static async Task IdentifyStaleIssues(GitHubEventClient gitHubEventClient { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.IdentifyStaleIssues)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); List includeLabels = new List { @@ -421,7 +433,7 @@ public static async Task IdentifyStaleIssues(GitHubEventClient gitHubEventClient int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; for (request.Page = 1; request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); @@ -480,6 +492,7 @@ public static async Task LockClosedIssues(GitHubEventClient gitHubEventClient, S { if (gitHubEventClient.RulesConfiguration.RuleEnabled(RulesConstants.LockClosedIssues)) { + int ScheduledTaskUpdateLimit = await gitHubEventClient.ComputeScheduledTaskUpdateLimit(); SearchIssuesRequest request = gitHubEventClient.CreateSearchRequest( scheduledEventPayload.Repository.Owner.Login, @@ -492,7 +505,7 @@ public static async Task LockClosedIssues(GitHubEventClient gitHubEventClient, S int numUpdates = 0; // In theory, maximumPage will be 10 since there's 100 results per-page returned by default but // this ensures that if we opt to change the page size - int maximumPage = MaxSearchResults / request.PerPage; + int maximumPage = RateLimitConstants.SearchIssuesRateLimit / request.PerPage; for (request.Page = 1; request.Page <= maximumPage; request.Page++) { SearchIssuesResult result = await gitHubEventClient.QueryIssues(request); diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs index 2fa7fa29e80..3cd384239a8 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs @@ -125,6 +125,8 @@ public GitHubIssueToUpdate(long repositoryId, int issueOrPRNumber, IssueUpdate i // for action processing which uses a shared event. protected List _gitHubIssuesToUpdate = new List(); + public int CoreRateLimit { get; set; } = 0; + public RulesConfiguration RulesConfiguration { get { return _rulesConfiguration; } @@ -267,6 +269,7 @@ public int ComputeNumberOfExpectedUpdates() public async Task WriteRateLimits(string prependMessage = null) { var miscRateLimit = await GetRateLimits(); + CoreRateLimit = miscRateLimit.Resources.Core.Limit; // Get the Minutes till reset. TimeSpan span = miscRateLimit.Resources.Core.Reset.UtcDateTime.Subtract(DateTime.UtcNow); // In the message, cast TotalMinutes to an int to get a whole number of minutes. @@ -279,10 +282,37 @@ public async Task WriteRateLimits(string prependMessage = null) } /// - /// Write the current rate limit and remaining number of transactions. + /// Return the number of updates a scheduled task can make. The Core Rate Limit that GitHub Actions can make is 15000/hour + /// for enterprise and 1000/hour for non-enterprise. The max number of results that can be retried from SearchIssues is 1000. + /// The CoreRateLimit is set when WriteRateLimits is called and this is done at the start of processing in Main. If the core + /// rate limit is 15000, return 1000, otherwise return 100 which is 1/10th of the hourly limit for non-enterprise repository. /// - /// Optional message to prepend to the rate limit message. - public async Task WriteSearchRateLimits(string prependMessage = null) + /// The number updates a scheduled task can make. + public virtual async Task ComputeScheduledTaskUpdateLimit() + { + // CoreRateLimit will be set in WriteRateLimits but if that hasn't been called yet, call it now. + if (CoreRateLimit == 0) + { + var miscRateLimit = await GetRateLimits(); + CoreRateLimit = miscRateLimit.Resources.Core.Limit; + } + // If the repository is an enterprise repository allow the max number search results, 1000, to be processed. + if (CoreRateLimit == RateLimitConstants.ActionRateLimitEnterprise) + { + return RateLimitConstants.SearchIssuesRateLimit; + } + else + { + // For non-enterprise return 1/10th of the non-enterprise rate limit + return RateLimitConstants.ActionRateLimitNonEnterprise/10; + } + } + + /// + /// Write the current rate limit and remaining number of transactions. + /// + /// Optional message to prepend to the rate limit message. + public async Task WriteSearchRateLimits(string prependMessage = null) { var miscRateLimit = await GetRateLimits(); // Get the Seconds till reset. Unlike the core rate limit which resets every hour, the search rate limit From 272356bbcfe3a049c7cc4cc9dcaa9d842af91146 Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 24 Mar 2023 12:27:03 -0700 Subject: [PATCH 2/3] Updates for feedback --- .../GitHubEventClient.cs | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs index 3cd384239a8..01094a4fbb7 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs @@ -290,29 +290,28 @@ public async Task WriteRateLimits(string prependMessage = null) /// The number updates a scheduled task can make. public virtual async Task ComputeScheduledTaskUpdateLimit() { + int updateLimit = 0; + // CoreRateLimit will be set in WriteRateLimits but if that hasn't been called yet, call it now. if (CoreRateLimit == 0) { var miscRateLimit = await GetRateLimits(); CoreRateLimit = miscRateLimit.Resources.Core.Limit; } - // If the repository is an enterprise repository allow the max number search results, 1000, to be processed. - if (CoreRateLimit == RateLimitConstants.ActionRateLimitEnterprise) + updateLimit = CoreRateLimit / 10; + if (updateLimit > RateLimitConstants.SearchIssuesRateLimit) { - return RateLimitConstants.SearchIssuesRateLimit; - } - else - { - // For non-enterprise return 1/10th of the non-enterprise rate limit - return RateLimitConstants.ActionRateLimitNonEnterprise/10; + updateLimit = RateLimitConstants.SearchIssuesRateLimit; } + Console.WriteLine($"Setting the scheduled task update limit to: {updateLimit}"); + return updateLimit; } - /// - /// Write the current rate limit and remaining number of transactions. - /// - /// Optional message to prepend to the rate limit message. - public async Task WriteSearchRateLimits(string prependMessage = null) + /// + /// Write the current rate limit and remaining number of transactions. + /// + /// Optional message to prepend to the rate limit message. + public async Task WriteSearchRateLimits(string prependMessage = null) { var miscRateLimit = await GetRateLimits(); // Get the Seconds till reset. Unlike the core rate limit which resets every hour, the search rate limit From 02f4dc41580fad16b1a7325d042e8f930d5883fe Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 24 Mar 2023 12:35:25 -0700 Subject: [PATCH 3/3] remove a couple of unused constants --- .../MockGitHubEventClient.cs | 4 ++-- .../Constants/RateLimitConstants.cs | 6 ------ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs index 25f560c48c1..ef6d16fb98e 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs @@ -458,14 +458,14 @@ public List GetGitHubIssuesToLock() /// /// Override for the GitHubEventClient funcion which computes this based upon the repository's core rate limit. - /// Since this isn't necessary for the tests, just return the ActionRateLimitNonEnterprise/10. + /// Since this isn't necessary for the tests, just return the 100 which is the size of one page. /// /// int #pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously public override async Task ComputeScheduledTaskUpdateLimit() #pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously { - return RateLimitConstants.ActionRateLimitNonEnterprise/10; + return 100; } } } diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs index 65c08df942f..e524e7bf4be 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs @@ -8,12 +8,6 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Constants { public class RateLimitConstants { - // https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions - // When using the GITHUB_TOKEN the number of Actions per hour for an Enterprise repository is 15000/hour - public const int ActionRateLimitEnterprise = 15000; - // When using the GITHUB_TOKEN the number of Actions per hour for an Non-enterprise repository is 1000/hour - public const int ActionRateLimitNonEnterprise = 1000; - // https://docs.github.com/en/rest/search?apiVersion=2022-11-28#about-search // The SearchIssues API has a rate limit of 1000 results which resets every 60 seconds public const int SearchIssuesRateLimit = 1000;