Rate limit support #165
Rate limit support #165
Conversation
ggreer
force-pushed
the
ggreer/rate-limits
branch
2 times, most recently
from
ee25737 to
2aa8a42
Compare
|
Warning Rate limit exceeded@ggreer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve modifications to error handling and response structures across multiple files. Key updates include the introduction of structured response variables in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Syncer
participant Annotations
participant HttpClient
Client->>Syncer: SyncResources()
Syncer->>Annotations: GetAnnotations()
Syncer->>HttpClient: DoRequest()
HttpClient-->>Syncer: Response
Syncer->>Annotations: ProcessResponse()
Syncer-->>Client: Return Annotations and Error
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/connectorbuilder/connectorbuilder.go (3 hunks)
- pkg/sync/syncer.go (34 hunks)
- pkg/uhttp/wrapper.go (1 hunks)
Additional comments not posted (14)
pkg/uhttp/wrapper.go (1)
177-179: LGTM!The new conditional check for handling server errors (status codes 500-599) is a good addition. It improves the error handling by:
- Distinguishing between client-side and server-side errors.
- Returning a specific error code (
codes.Unavailable) for server errors, enhancing the granularity of error reporting.- Including the actual HTTP response status in the error message, providing more context about the error.
The code changes are approved.
pkg/connectorbuilder/connectorbuilder.go (3)
351-366: LGTM!The code changes introduce a new response variable
respthat is constructed as a pointer tov2.ResourcesServiceListResourcesResponsestruct. This ensures that a structured response is returned, even in the event of an error, providing additional context to the caller. The error messages remain largely unchanged, but they now accompany the structured response.The final return statement has been simplified to return the
respvariable along with anilerror, replacing the previous inline construction of the response struct. This enhances code readability and maintainability.
383-398: LGTM!The code changes in
ListEntitlementsfollow the same pattern asListResources. A new response variablerespis introduced, which is constructed as a pointer tov2.EntitlementsServiceListEntitlementsResponsestruct. This ensures that a structured response is returned, even in the event of an error, providing additional context to the caller. The error messages remain largely unchanged, but they now accompany the structured response.The final return statement has been simplified to return the
respvariable along with anilerror, replacing the previous inline construction of the response struct. This enhances code readability and maintainability.
416-433: LGTM!The code changes in
ListGrantsfollow the same pattern asListResourcesandListEntitlements. A new response variablerespis introduced, which is constructed as a pointer tov2.GrantsServiceListGrantsResponsestruct. This ensures that a structured response is returned, even in the event of an error, providing additional context to the caller. The error messages remain largely unchanged, but they now accompany the structured response.The final return statement has been simplified to return the
respvariable along with anilerror, replacing the previous inline construction of the response struct. This enhances code readability and maintainability.pkg/sync/syncer.go (10)
Line range hint
87-120: LGTM!The code changes in
shouldWaitAndRetrylook good:
- The function correctly handles the new
annosparameter.- It extracts the rate limit data from annotations to determine the wait duration.
- It falls back to the default linear backoff if annotations are missing.
The changes are approved.
Line range hint
125-268: LGTM!The code changes in
Synclook good:
- The function correctly handles the new return types from the synchronization functions.
- It captures the returned annotations and passes them to
shouldWaitAndRetryfor informed retry decisions.The changes are approved.
Line range hint
280-315: LGTM!The code changes in
SyncResourceTypeslook good:
- The function correctly returns the annotations from the connector response.
- It handles the case when the connector returns an error and returns the annotations along with the error.
The changes are approved.
Line range hint
343-373: LGTM!The code changes in
SyncResourceslook good:
- The function correctly returns the annotations from the
syncResourcesfunction or the store'sListResourceTypesmethod.- It handles the case when an error occurs and returns the annotations along with the error.
The changes are approved.
Line range hint
377-442: LGTM!The code changes in
syncResourceslook good:
- The function correctly returns the annotations from the connector response.
- It handles the case when the connector returns an error and returns the annotations along with the error.
The changes are approved.
Line range hint
509-555: LGTM!The code changes in
SyncEntitlementslook good:
- The function correctly returns the annotations from the store's
ListResourcesmethod when fetching the resources.- It returns
nilannotations when syncing entitlements for a specific resource.The changes are approved.
Line range hint
701-741: LGTM!The code changes in
SyncAssetslook good:
- The function correctly returns the annotations from the store's
ListResourcesmethod when fetching the resources.- It returns
nilannotations when syncing assets for a specific resource.The changes are approved.
Line range hint
746-859: LGTM!The code changes in
SyncGrantExpansionlook good:
- The function correctly returns the annotations from the store's
ListGrantsmethod when fetching the grants.- It returns
nilannotations when expanding grants for entitlements.The changes are approved.
Line range hint
864-910: LGTM!The code changes in
SyncGrantslook good:
- The function correctly returns the annotations from the store's
ListResourcesmethod when fetching the resources.- It returns the annotations from
syncGrantsForResourcewhen syncing grants for a specific resource.The changes are approved.
Line range hint
1034-1126: LGTM!The code changes in
syncGrantsForResourcelook good:
- The function correctly returns the annotations from the store's
GetResourcemethod when fetching the resource.- It returns the annotations from the connector's
ListGrantsmethod when listing the grants.- It handles the case when an error occurs and returns the annotations along with the error.
The changes are approved.
shackra
force-pushed
the
ggreer/rate-limits
branch
from
b2ef106 to
d2cb3e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (5)
pkg/sync/syncer.go (5)
Line range hint
327-362: RefactorSyncResourceTypesto Handle Pagination and Errors More EffectivelyThe
SyncResourceTypesfunction has been refactored to handle pagination and errors effectively. Here are some points for improvement:
- Error Handling: The function now returns annotations along with errors, which is great for detailed error context. Ensure that all possible error paths are tested, especially with non-standard API responses.
- Pagination Logic: The handling of pagination through
NextPageTokenis crucial for syncing large datasets. Ensure that this logic is robust and handles edge cases, such as API changes or unexpected response formats.- Logging and Monitoring: Consider adding more detailed logging at each step of the pagination and error handling. This can help in quick diagnosis during issues in production environments.
Consider adding unit tests that mock various API response scenarios to ensure that the pagination and error handling work as expected under different conditions.
Line range hint
390-489: OptimizeSyncResourcesfor Performance and ClarityThe
SyncResourcesfunction is critical for syncing resource data. While the updates are in line with the PR's objectives, here are some suggestions for further refinement:
- Performance: The function handles pagination and recursive resource fetching, which can be performance-intensive. Consider optimizing the data fetching and processing patterns, possibly by batching requests or parallelizing independent operations.
- Clarity and Maintenance: The function's logic is somewhat complex due to the recursive nature and multiple conditional branches. Refactoring for clarity, possibly by breaking down the function into smaller, more focused sub-functions, could improve maintainability.
- Error Handling: Enhanced error handling with detailed annotations helps in understanding the context of failures. Ensure comprehensive coverage of error scenarios in unit tests to prevent runtime issues.
Overall, the function's implementation aligns with the intended enhancements in error handling and rate limit respect. Further optimizations could help in maintaining high performance and reliability.
Line range hint
556-602: ImproveSyncEntitlementswith Clear Error Handling and Efficient PaginationThe
SyncEntitlementsfunction has been updated to handle entitlements syncing with detailed annotations and error handling. Here are some areas for improvement:
- Error Handling: The function now returns detailed error information along with annotations, which is excellent for debugging and error tracking. Ensure that all error paths are thoroughly tested.
- Pagination Handling: The function deals with pagination, which is essential for handling large datasets. Ensure that the pagination logic is robust and handles all edge cases, such as incomplete pages or API changes.
- Code Clarity: Given the complexity introduced by error handling and pagination, consider refactoring the function to improve readability and maintainability. Breaking down complex logic into smaller, well-named functions could help.
Adding more detailed logging and monitoring at critical steps can also help in quicker issue resolution during production incidents.
Line range hint
748-788: RefactorSyncAssetsfor Better Error Handling and ClarityThe
SyncAssetsfunction has been updated to handle asset synchronization with enhanced error handling and annotations. Here are some suggestions for improvement:
- Error Handling: The function's approach to handling errors and returning annotations is a step in the right direction for better error context. Ensure that all potential error scenarios are covered in tests.
- Clarity and Maintenance: The function's logic around pagination and error handling could be clearer. Consider refactoring for better readability and maintainability, possibly by extracting some logic into helper functions.
- Performance Considerations: Given the potentially large number of assets to sync, consider performance optimizations such as request batching or parallel processing.
Overall, the updates are aligned with the PR's objectives, and further refinements could enhance the function's effectiveness and reliability.
Line range hint
793-906: EnhanceSyncGrantExpansionwith Robust Error Handling and Logical FlowThe
SyncGrantExpansionfunction has been significantly updated to handle grant expansion with detailed annotations and robust error handling. Here are some points for further enhancement:
- Error Handling: The detailed error handling and use of annotations provide excellent visibility into the function's operation. Ensure that all error paths are well-tested, especially in scenarios where annotations might be missing or incomplete.
- Logical Flow: The function handles complex logic related to grant expansion, including handling cycles and conditional processing based on the state of the entitlement graph. Consider simplifying or documenting this logic to improve understandability and maintainability.
- Performance Optimization: Given the potentially intensive operations involved in grant expansion, consider optimizations such as caching frequently accessed data or parallelizing independent operations.
Refactoring for clarity and adding comprehensive unit tests can help ensure that the function performs as expected under various scenarios.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/connectorbuilder/connectorbuilder.go (3 hunks)
- pkg/sync/syncer.go (34 hunks)
- pkg/uhttp/wrapper.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/connectorbuilder/connectorbuilder.go
- pkg/uhttp/wrapper.go
Additional comments not posted (1)
pkg/sync/syncer.go (1)
Line range hint
200-262: Enhanced Error Handling and Retry Logic in Sync OperationsThe synchronization functions such as
SyncResourceTypes,SyncResources, etc., have been updated to return both annotations and errors. This change is crucial for implementing the retry logic based on rate limit information provided in the annotations.
- Unified Error Handling: It's good to see a consistent approach to handling retries across different sync operations. This makes the code easier to maintain and understand.
- Annotations Usage: The use of annotations to decide on retries is a significant improvement. It allows the system to be more responsive to backend server states and rate limiting, which can help prevent overloading the server and improve sync reliability.
- Potential Refinement: While the current implementation covers the basic needs, consider abstracting some of the repetitive code into a helper function or middleware that handles retries. This could reduce code duplication and centralize the retry logic.
Overall, these changes are well-aligned with the PR's goals and should contribute to a more robust synchronization process.
pkg/sync/syncer.go
Outdated
| func shouldWaitAndRetry(ctx context.Context, annos annotations.Annotations, err error) bool { | ||
| if err == nil { | ||
| attempts = 0 | ||
| return true | ||
| } | ||
| if status.Code(err) != codes.Unavailable { | ||
| if status.Code(err) != codes.Unavailable && status.Code(err) != codes.DeadlineExceeded { | ||
| return false | ||
| } | ||
|
|
||
| attempts++ | ||
| l := ctxzap.Extract(ctx) | ||
|
|
||
| // use lineal time by default | ||
| var wait time.Duration = time.Duration(attempts) * time.Second | ||
| rlData := &v2.RateLimitDescription{} | ||
| if annos != nil { | ||
| ok, err := annos.Pick(rlData) | ||
| if ok && err == nil { | ||
| // or use time provided in the annotations | ||
| wait = time.Until(rlData.ResetAt.AsTime()) | ||
| } | ||
| } | ||
|
|
||
| l.Error("retrying operation", zap.Error(err), zap.Duration("wait", wait)) | ||
| l.Error("RETRYING OPERATION", zap.Error(err), zap.Duration("wait", wait), zap.Any("rate_limit", rlData)) |
There was a problem hiding this comment.
Refactor suggestion for shouldWaitAndRetry function
The shouldWaitAndRetry function has been updated to include rate limit handling using annotations. Here are a few suggestions and observations:
- Error Handling: The function now handles
UnavailableandDeadlineExceedederrors specifically, which is a good practice for retry mechanisms. However, it might be beneficial to expand this to other transient errors that could also benefit from retries. - Rate Limit Handling: The use of rate limit data from annotations is a robust way to respect server-specified backoff times. Ensure that the
RateLimitDescriptionis being parsed correctly and that theResetAtfield is always present when expected. - Logging: The error logging before entering the retry loop is helpful for debugging. Consider adding more detailed logs that could help trace the values of variables more precisely, especially around the rate limit data.
- Infinite Loop Risk: The function uses a
forloop with aselectstatement for retrying, which is generally safe. However, ensure there's a mechanism to break out of the loop in case of unexpected conditions to prevent potential infinite loops.
Overall, the changes are aligned with the PR's objectives to handle rate limits more effectively. Consider adding more comprehensive tests to cover various scenarios of rate limit headers and error types.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pkg/sync/syncer.go (34 hunks)
- pkg/uhttp/wrapper.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/sync/syncer.go
- pkg/uhttp/wrapper.go
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
pkg/sync/syncer.go (2)
Line range hint
250-262: Enhanced error handling inSyncAssetsandSyncGrantExpansionThe modifications in the
SyncAssetsandSyncGrantExpansionfunctions include improved error handling by integrating annotations into the retry logic. This is a significant improvement as it allows these functions to provide additional context about the operation's outcome, which can be leveraged in the retry logic.
SyncAssets: The function now correctly handles errors by retrying based on the annotations provided. This is crucial for operations that may be rate-limited or face temporary issues.SyncGrantExpansion: Similarly, this function uses the enhanced retry logic to handle errors more effectively. The detailed debug logs added in this function will aid in troubleshooting and ensuring that the grant expansion process is robust.These changes are in line with the PR's objectives to improve the clarity of the code and potentially increase the reliability of the synchronization operations.
Line range hint
950-1174: Review ofsyncGrantsForResourcefunctionThe
syncGrantsForResourcefunction has undergone significant changes to include annotations in its return type and to enhance error handling. Here are some specific observations:
- Annotations Handling: The function now returns annotations along with the error, which is a good practice as it provides more context about the error or the state of the operation.
- Error Handling: The function's error handling has been improved by checking for specific conditions and returning detailed errors. This makes the function more robust and easier to debug.
- Integration with ETag Logic: The integration of ETag logic to handle conditional requests based on previous sync states is a sophisticated feature that can reduce unnecessary data transfer and processing. This is particularly useful in environments where data changes infrequently.
- Potential Improvements:
- Error Handling Consistency: Ensure that all potential error paths are handled consistently and that errors are logged with sufficient detail.
- Testing: Given the complexity of the function, especially with the integration of ETag logic, comprehensive testing is crucial to ensure that all edge cases are covered.
Overall, the changes enhance the function's capability to handle synchronization tasks more effectively, aligning with the PR's goals.
pkg/sync/syncer.go
Outdated
| if status.Code(err) != codes.Unavailable && status.Code(err) != codes.DeadlineExceeded { | ||
| return false | ||
| } | ||
|
|
||
| attempts++ | ||
| l := ctxzap.Extract(ctx) | ||
|
|
||
| // use lineal time by default | ||
| var wait time.Duration = time.Duration(attempts) * time.Second | ||
| if st, ok := status.FromError(err); ok { | ||
| details := st.Details() | ||
| for _, detail := range details { | ||
| if rlData, ok := detail.(*v2.RateLimitDescription); ok { | ||
| wait = time.Until(rlData.ResetAt.AsTime()) | ||
| } | ||
| } | ||
| l.Debug("details from status error", zap.Any("details", details), zap.Int("len of details", len(details)), zap.Error(err)) | ||
| } else { | ||
| l.Debug("unable to parse rate limit description from error", zap.Error(err), zap.Any("status", st)) | ||
| } | ||
|
|
||
| l.Error("retrying operation", zap.Error(err), zap.Duration("wait", wait)) | ||
| l.Error("RETRYING OPERATION", zap.Error(err), zap.Duration("wait", wait)) |
There was a problem hiding this comment.
Refactor suggestion for shouldWaitAndRetry function
The shouldWaitAndRetry function has been updated to include rate limit handling using annotations. Here are a few suggestions and observations:
- Error Handling: The function now handles
UnavailableandDeadlineExceedederrors specifically, which is a good practice for retry mechanisms. However, it might be beneficial to expand this to other transient errors that could also benefit from retries. - Rate Limit Handling: The use of rate limit data from annotations is a robust way to respect server-specified backoff times. Ensure that the
RateLimitDescriptionis being parsed correctly and that theResetAtfield is always present when expected. - Logging: The error logging before entering the retry loop is helpful for debugging. Consider adding more detailed logs that could help trace the values of variables more precisely, especially around the rate limit data.
- Infinite Loop Risk: The function uses a
forloop with aselectstatement for retrying, which is generally safe. However, ensure there's a mechanism to break out of the loop in case of unexpected conditions to prevent potential infinite loops.
Overall, the changes are aligned with the PR's objectives to handle rate limits more effectively. Consider adding more comprehensive tests to cover various scenarios of rate limit headers and error types.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pkg/uhttp/transport.go (1 hunks)
- pkg/uhttp/wrapper.go (3 hunks)
Files skipped from review due to trivial changes (1)
- pkg/uhttp/transport.go
Files skipped from review as they are similar to previous changes (1)
- pkg/uhttp/wrapper.go
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
pkg/sync/syncer.go (3)
95-117: Approved with suggestionsThe changes to the
shouldWaitAndRetryfunction look good! Parsing the rate limit data from the error details to determine the retry wait duration is a nice enhancement.Suggestions:
- Consider providing a more detailed error message when unable to parse the rate limit description from the error. This will aid in debugging such scenarios.
- Add unit tests to cover various scenarios of rate limit headers and error types to ensure the robustness of the retry logic.
1084-1089: Approved with suggestionThe new
SyncGrantExpansionfunction looks good! It aligns with the PR objective of enhancing the grant expansion process.Suggestion:
- Add documentation to explain the purpose and behavior of this function. This will help future maintainers understand its role in the grant expansion process.
Line range hint
1104-1176: Approved with suggestionThe changes to the
syncGrantsForResourcefunction look good! Fetching and processing etagged grants before fetching new grants, and updating the resource etag based on the etag match or response etag, are nice enhancements to the grant fetching process.Suggestion:
- Consider adding more detailed logging statements to help trace the values of variables more precisely, especially around the etag handling logic. This will aid in debugging and understanding the flow of the function.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- pkg/sync/syncer.go (10 hunks)
- pkg/uhttp/wrapper.go (3 hunks)
Additional context used
golangci-lint
pkg/uhttp/wrapper.go
362-362: ineffectual assignment to err
(ineffassign)
GitHub Check: go-lint
pkg/uhttp/wrapper.go
[failure] 362-362:
ineffectual assignment to err (ineffassign)
Additional comments not posted (5)
pkg/uhttp/wrapper.go (2)
217-229: LGTM!The new
wrapRetryAfterInStatusfunction looks good. It correctly uses theratelimit.ExtractRateLimitDatafunction to handle different rate limit headers, addressing the previous comment.
286-366: Great job on enhancing the error handling!The changes to the
Dofunction make the error handling more robust by providing clearer feedback on rate limiting issues. This addresses the previous comment to make the code as general as possible.Tools
golangci-lint
362-362: ineffectual assignment to err
(ineffassign)
GitHub Check: go-lint
[failure] 362-362:
ineffectual assignment to err (ineffassign)pkg/sync/syncer.go (3)
252-252: ApprovedThe changes to the
SyncResourceTypesfunction look good! Returning annotations along with the error allows for providing additional context about the operation's outcome, which is a nice improvement.
264-264: ApprovedThe changes to the
SyncAssetsfunction look good! Returning annotations alongside errors allows for providing additional context about the operation's outcome, which is a nice improvement.
1343-1343: ApprovedThe new
newExpandedGrantfunction looks good! It aligns with the PR objective of enhancing the grant expansion process by creating a new grant with the provided entitlement and principal.
pkg/uhttp/wrapper.go
Outdated
| l.Debug("error setting cache", zap.String("cacheKey", cacheKey), zap.String("url", req.URL.String()), zap.Error(err)) | ||
| return resp, err | ||
| l.Warn("error setting cache", zap.String("cacheKey", cacheKey), zap.String("url", req.URL.String()), zap.Error(err)) | ||
| err = nil |
There was a problem hiding this comment.
Fix the ineffectual assignment to err.
The static analysis tools have correctly flagged an ineffectual assignment to err at line 362.
To fix this, remove the assignment to err:
- err = nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = nil |
Tools
golangci-lint
362-362: ineffectual assignment to err
(ineffassign)
GitHub Check: go-lint
[failure] 362-362:
ineffectual assignment to err (ineffassign)
…in rate limiting.
ggreer
force-pushed
the
ggreer/rate-limits
branch
from
11b40e8 to
4eaa9a4
Compare
…e's an issue geting rate limit data or adding details.
… probably happens a lot because not all http servers return rate limit data.
The current behavior is to retry recoverable errors every second and linearly backoff. This adds backoff that respects rate limit annotations.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes