chore: Skip unnecessary RTS calls to optimise performance#37949
chore: Skip unnecessary RTS calls to optimise performance#37949
Conversation
|
Warning Rate limit exceeded@subrata71 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 58 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 made to the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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
Documentation and Community
|
github-actions
bot
added
AST-backend
![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
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java (1)
146-156: Consider adding performance metrics for skipped calls.While we have monitoring for slow RTS calls, it would be valuable to track how many calls we're avoiding with this optimization.
Add metrics before the early return:
if (!bindingValue.getValue().contains(oldName)) { + log.debug("Skipping RTS call for binding: {}", bindingValue); return Mono.just(Tuples.of(bindingValue, bindingValue.getValue())); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java (1)
147-155: Optimization looks good and aligns with PR objectives.
The early return for bindings that don't contain the oldName is a good optimization that will reduce unnecessary RTS calls. The example in the comments clearly illustrates the use case.
Let's verify the impact of this optimization:
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12164120141. |
|
Deploy-Preview-URL: https://ce-37949.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java (3)
24-34: Consider adding descriptive assertions for better test documentation.While the test correctly verifies empty results, adding assertNext with descriptive assertions would make the test's intent clearer.
- StepVerifier.create(result).verifyComplete(); + StepVerifier.create(result) + .expectNextCount(0) + .verifyComplete();
69-69: Fix misleading comment.The comment "Only one binding refactored" is incorrect as both bindings are being refactored in this test case.
- assertThat(map).hasSize(2); // Only one binding refactored + assertThat(map).hasSize(2); // Both bindings should be refactored
76-101: Consider extracting test strings as constants.For better maintainability and reusability, consider extracting the input and expected output scripts as private static constants.
+ private static final String INPUT_JS_SCRIPT = "export default { myFun1() { Api1.run(); return Api1.data;}}"; + private static final String EXPECTED_JS_SCRIPT = "export default { myFun1() { GetUsers.run(); return GetUsers.data;}}";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java(1 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java (3)
1-23: LGTM! Well-structured test class setup.
The class is properly configured with Spring Boot test annotations and has all necessary dependencies.
36-54: LGTM! Test validates optimization for unchanged bindings.
This test case effectively validates the PR's objective of skipping unnecessary RTS calls when the oldName is not present in bindings.
103-127: LGTM! Critical test for optimization scenario.
This test effectively validates that the service returns the original script without processing when no matches are found, which is crucial for the performance optimization goal.
Failed server tests
|
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ASTServiceCETest.java
Outdated
Show resolved
Hide resolved
Failed server tests
|
…g#37949) ## Description Motivation came from this [PoC](https://www.notion.so/appsmith/Evaluating-Performance-Bottlenecks-in-Pull-Based-Upgrades-Challenges-with-New-Relic-Telemetry-14bfe271b0e28063b9fdc08515ab3014) Fixes appsmithorg#37948 ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12181005719> > Commit: d1f53e2 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12181005719&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 05 Dec 2024 15:10:54 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling in the binding refactoring process to ensure smoother operation. - **Refactor** - Enhanced efficiency of the binding refactoring method by adding a conditional check to reduce unnecessary server calls. - **Tests** - Introduced a new test suite for the binding refactoring method, validating various scenarios to ensure correct functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Motivation came from this PoC
Fixes #37948
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12181005719
Commit: d1f53e2
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Dec 2024 15:10:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor
Tests