fix: Reduce RTS calls in the updateLayout flow #37127
Conversation
WalkthroughThe changes in this pull request involve the addition of new constants and modifications to several classes within the Appsmith codebase. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
the
skip-changelog
Failed server tests
|
- After this change we reduce the RTS calls from 2*N to 2 where N is the no of bindings in the layout.
Failed server tests
|
github-actions
bot
added
Integrations Pod General
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
1 similar comment
Failed server tests
|
rishabhrathod01
changed the title
github-actions
bot
added
Bug
Failed server tests
|
Failed server tests
|
Failed server tests
|
1 similar comment
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java (1)
14-14: Consider adding documentation for monitoring points.Adding Javadoc comments for these new span constants would help other developers understand:
- The specific operations being monitored
- Their role in the RTS call optimization
- Expected impact on performance
+ /** + * Span for monitoring entity reference resolution operations. + * Added as part of RTS call optimization in the updateLayout flow. + */ public static final String GET_POSSIBLE_ENTITY_REFERENCES = APPSMITH_SPAN_PREFIX + "getPossibleEntityReferences"; + /** + * Span for monitoring entity parent relationship mapping operations. + * Used to optimize entity hierarchy traversal. + */ public static final String GET_POSSIBLE_ENTITY_PARENTS_MAP = APPSMITH_SPAN_PREFIX + "getPossibleEntityParentsMap"; + /** + * Span for monitoring dynamic binding reference resolution. + * Helps track and optimize binding-related RTS calls. + */ public static final String GET_POSSIBLE_REFERENCES_FROM_DYNAMIC_BINDING = APPSMITH_SPAN_PREFIX + "getPossibleReferencesFromDynamicBinding";Also applies to: 17-17, 23-24
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java (1)
Line range hint
55-55: Review performance monitoring thresholdsGiven that RTS calls currently take ~3s, the MAX_API_RESPONSE_TIME_IN_MS threshold of 50ms seems too aggressive. Additionally, using debug level for logging these performance issues might make them harder to track.
Consider these improvements:
- Adjust MAX_API_RESPONSE_TIME_IN_MS to a more realistic threshold based on current metrics
- Upgrade to log.warn for better visibility of performance issues
- Add metrics collection for these slow calls
- private static final long MAX_API_RESPONSE_TIME_IN_MS = 50; + private static final long MAX_API_RESPONSE_TIME_IN_MS = 1000; // 1s threshold // In elapsed().map block: - log.debug("Time elapsed since AST refactor call: {} ms", tuple.getT1()); - log.debug("This call took longer than expected. The binding was: {}", bindingValue); + log.warn("Time elapsed since AST refactor call: {} ms", tuple.getT1()); + log.warn("This call took longer than expected. The binding was: {}", bindingValue);Also applies to: 169-173
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java (1)
ObservationRegistry is declared but not utilized in MustacheHelper.java
- The
observationRegistryis initialized but not used in any methods within the file.🔗 Analysis chain
Line range hint
39-79: Verify the integration of ObservationRegistryThe ObservationRegistry is added but not utilized in any of the methods. This seems incomplete and doesn't align with the PR objective of reducing RTS calls.
Let's check for other files that might be using this registry:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ObservationRegistry usage in the codebase rg -l "ObservationRegistry" --type java # Search for similar observation patterns ast-grep --pattern 'new Observation.Context($_, $_)'Length of output: 6034
app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (1)
601-686: Avoid code duplication in overloaded methodsThe overloaded
getPossibleEntityReferencesMapmethods share significant code. Refactoring to eliminate duplication will improve maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/MustacheHelper.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/AstServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java(5 hunks)
🔇 Additional comments (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/AstServiceImpl.java (2)
7-7: LGTM: Clean import addition
The ObservationRegistry import is properly placed with other framework imports.
15-20: LGTM: Constructor properly updated for observability
The constructor modification correctly integrates ObservationRegistry for monitoring capabilities. This aligns with the PR's goal of optimizing RTS calls.
Let's verify the superclass implementation:
✅ Verification successful
ObservationRegistry integration in superclass confirmed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the superclass constructor and its usage of ObservationRegistry
# Check the superclass implementation
ast-grep --pattern 'class AstServiceCEImpl {
$$$
private final ObservationRegistry observationRegistry;
$$$
AstServiceCEImpl($$$) {
$$$
}
$$$
}'
Length of output: 162
Script:
#!/bin/bash
# Description: Find the superclass of AstServiceImpl
rg 'class AstServiceImpl\s+extends\s+(\w+)' --only-matching --replace '$1' app/server/appsmith-server/src/main/java/com/appsmith/server/services/AstServiceImpl.java
Length of output: 183
Script:
#!/bin/bash
# Description: Check if ObservationRegistry is used in AstServiceCEImpl
rg 'ObservationRegistry' app/server/appsmith-server/src/main/java/com/appsmith/server/services/AstServiceCEImpl.java
Length of output: 248
Script:
#!/bin/bash
# Description: Locate AstServiceCEImpl.java in the repository
fd 'AstServiceCEImpl\.java' .
Length of output: 128
Script:
#!/bin/bash
# Description: Check if ObservationRegistry is used in the correct AstServiceCEImpl.java
rg 'ObservationRegistry' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java
Length of output: 232
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java (1)
14-14: LGTM! Constants align with performance optimization goals.
The new span constants follow the established naming pattern and provide monitoring points for entity reference operations, which aligns with the PR's objective of reducing RTS calls.
Also applies to: 17-17, 23-24
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java (2)
11-11: LGTM: Clean integration of monitoring capabilities
The addition of ObservationRegistry is well-structured and follows proper dependency injection patterns.
Also applies to: 20-20, 45-45
133-134: LGTM: Well-placed observation tap
The Micrometer observation tap is correctly positioned at the end of the reactive chain, enabling proper monitoring of RTS call performance.
app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (2)
675-676:
Potential issue with map updates inside loops
Updating possibleEntitiesReferencesToBindingMap inside the inner loop may lead to incorrect mappings. Move the map update outside the inner loop to ensure correct aggregation.
Apply this diff to adjust the map update:
- possibleEntitiesReferencesToBindingMap.put(binding, possibleEntitiesReferences);
});
+ possibleEntitiesReferencesToBindingMap.put(binding, possibleEntitiesReferences);Likely invalid or redundant comment.
513-518:
Syntax error: Incomplete method declaration
The method getPossibleEntityReferences is missing its parameter list and body.
Apply this diff to fix the syntax error:
-protected Mono<Set<EntityDependencyNode>> getPossibleEntityReferences(
-
- return getPossibleEntityReferences(executableNameToExecutableMapMono, bindings, evalVersion, null)
- .name(GET_POSSIBLE_ENTITY_REFERENCES)
- .tap(Micrometer.observation(observationRegistry));
+protected Mono<Set<EntityDependencyNode>> getPossibleEntityReferences(
+ Mono<Map<String, Executable>> executableNameToExecutableMapMono, Set<String> bindings, int evalVersion) {
+ return getPossibleEntityReferences(executableNameToExecutableMapMono, bindings, evalVersion, null)
+ .name(GET_POSSIBLE_ENTITY_REFERENCES)
+ .tap(Micrometer.observation(observationRegistry));
+}Likely invalid or redundant comment.
|
|
||
| // TODO: add error handling scenario for when RTS is not accessible in fat container | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
TODO for RTS Accessibility Error Handling Not Addressed
The error handling scenario for RTS accessibility in AstServiceCEImpl.java remains unimplemented.
- File:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java
🔗 Analysis chain
Address the TODO comment regarding RTS accessibility
The TODO comment about error handling for RTS inaccessibility in fat container remains unaddressed. This could be critical for reliability.
Let's check if this scenario is handled elsewhere:
Would you like me to help implement the error handling for this scenario?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RTS accessibility handling patterns
rg -l "getIsRtsAccessible|RTS.*accessibility" --type java
Length of output: 575
Script:
#!/bin/bash
# Check for error handling related to RTS accessibility in AstServiceCEImpl.java
rg "RTS.*(error handling|exception)" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AstServiceCEImpl.java
Length of output: 133
| private static ObservationRegistry observationRegistry; | ||
|
|
There was a problem hiding this comment.
Static field initialization through instance constructor is an anti-pattern
The observationRegistry is declared as static but initialized through an instance constructor. This creates potential thread-safety issues and makes the initialization order unpredictable when multiple instances are created.
Consider this alternative implementation:
-private static ObservationRegistry observationRegistry;
+private final ObservationRegistry observationRegistry;
public MustacheHelper(ObservationRegistry observationRegistry) {
- this.observationRegistry = observationRegistry;
+ this.observationRegistry = observationRegistry;
}Also applies to: 77-79
| private Mono<Map<String, Set<EntityDependencyNode>>> getPossibleEntityReferencesMap( | ||
| Mono<Map<String, Executable>> executableNameToExecutableMapMono, Set<String> bindings, int evalVersion) { | ||
| return getPossibleEntityReferencesMap(executableNameToExecutableMapMono, bindings, evalVersion, null) | ||
| .name(GET_POSSIBLE_ENTITY_REFERENCES) | ||
| .tap(Micrometer.observation(observationRegistry)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce code duplication
The method getPossibleEntityReferencesMap duplicates logic from its overloaded version. Consider consolidating common code into a helper method.
Description
Fixes #37055
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/11804864728
Commit: 2b0981b
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 12 Nov 2024 22:04:29 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation