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.

Sign up for GitHub

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

Jump to bottom

[Task]: Resolve the delay due to RTS for the updateLayout #37055

Closed
NilanshBansal opened this issue Oct 24, 2024 · 1 comment · Fixed by #37127
Closed

[Task]: Resolve the delay due to RTS for the updateLayout #37055

NilanshBansal opened this issue Oct 24, 2024 · 1 comment · Fixed by #37127
Assignees
@rishabhrathod01
Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Integrations Product Issues related to a specific integration Query & JS Pod Issues related to the query & JS Pod Task A simple Todo

Comments

@NilanshBansal
Copy link
Contributor

SubTasks

Post adding instrumentation around the JS object updation request, we observed from the metrics that the RTS call is taking about 3 seconds for every request.
This newrelic trace verifies this information.

image

This task is to further debug and understand the reason for this large time taken.
@NilanshBansal NilanshBansal added Task A simple Todo Query & JS Pod Issues related to the query & JS Pod Integrations Product Issues related to a specific integration Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod labels Oct 24, 2024
@NilanshBansal NilanshBansal self-assigned this Oct 24, 2024
@NilanshBansal
Copy link
Contributor Author

NilanshBansal commented Oct 24, 2024 •
edited
Loading

As per discussion within the team, I am moving primarily to working on the Paragon POC. The task that I am starting with currently is #37058. Assigning this task to @rishabhrathod01
cc: @carinanfonseca @appsmithorg/query-js-pod

@rishabhrathod01 rishabhrathod01 mentioned this issue Oct 30, 2024
Merged
6 tasks
@rishabhrathod01 rishabhrathod01 changed the title [Task]: Debug why RTS is taking ~3s per request [Task]: Resolve the delay due to RTS for the updateLayout Nov 15, 2024
rishabhrathod01 pushed a commit that referenced this issue Nov 29, 2024
fix: Reduce RTS calls in the updateLayout flow (#37127)
e8cb73d 
## Description

In this PR, we update the code to send all the bindings to RTS together
instead of calling it for each property binding. It was observed that
the calls were earlier around 40k for the test application.

This change leads to optimisation of performance for 
- updateLayout call ( when updating anything in the DSL ) 
- query/API action update 
- JS action update

Fixes #37055



## Tests on DP 

Deploy-Preview-URL: https://ce-37127.dp.appsmith.com/

- [x] Multiple widgets with the same bindings impacting the sequence of
onPageLoadActions
- onPageLoadActions update as expected when
   - [x] DSL is updated
   - [x] Query is updated
   - [x] JS Object is updated

EE PR - https://github.com/appsmithorg/appsmith-ee/pull/5642 

## 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/12082854290>
> Commit: 07b68d4
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12082854290&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Fri, 29 Nov 2024 11:44:31 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

- **New Features**
- Enhanced retrieval of possible entity references and their
relationships with a new method.
- Added new test cases to validate action execution on page load and
dynamic binding handling.

- **Bug Fixes**
	- Improved error handling and validation for executable references.
	- Enhanced assertions to ensure correct execution of layout actions.

- **Tests**
- Added a test to validate layout actions with multiple widgets
referencing the same action.
	- Refactored existing tests for better readability and maintainability.
- Expanded test coverage for layout updates and dynamic binding
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this issue Dec 2, 2024
fix: Reduce RTS calls in the updateLayout flow (appsmithorg#37127)
155ea6a 
## Description

In this PR, we update the code to send all the bindings to RTS together
instead of calling it for each property binding. It was observed that
the calls were earlier around 40k for the test application.

This change leads to optimisation of performance for 
- updateLayout call ( when updating anything in the DSL ) 
- query/API action update 
- JS action update

Fixes appsmithorg#37055



## Tests on DP 

Deploy-Preview-URL: https://ce-37127.dp.appsmith.com/

- [x] Multiple widgets with the same bindings impacting the sequence of
onPageLoadActions
- onPageLoadActions update as expected when
   - [x] DSL is updated
   - [x] Query is updated
   - [x] JS Object is updated

EE PR - https://github.com/appsmithorg/appsmith-ee/pull/5642 

## 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/12082854290>
> Commit: 07b68d4
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12082854290&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Fri, 29 Nov 2024 11:44:31 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

- **New Features**
- Enhanced retrieval of possible entity references and their
relationships with a new method.
- Added new test cases to validate action execution on page load and
dynamic binding handling.

- **Bug Fixes**
	- Improved error handling and validation for executable references.
	- Enhanced assertions to ensure correct execution of layout actions.

- **Tests**
- Added a test to validate layout actions with multiple widgets
referencing the same action.
	- Refactored existing tests for better readability and maintainability.
- Expanded test coverage for layout updates and dynamic binding
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rishabhrathod01 rishabhrathod01

Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Pod Integrations Product Issues related to a specific integration Query & JS Pod Issues related to the query & JS Pod Task A simple Todo
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Reduce RTS calls in the updateLayout flow appsmithorg/appsmith
2 participants
@rishabhrathod01 @NilanshBansal