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]: Add metrics for uninstrumented time during action update #37114

Closed
sneha122 opened this issue Oct 28, 2024 · 1 comment · Fixed by #37125
Closed

[Task]: Add metrics for uninstrumented time during action update #37114

sneha122 opened this issue Oct 28, 2024 · 1 comment · Fixed by #37125
Assignees
@sneha122
Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Product Issues related to a specific integration Javascript Product Issues related to users writing javascript in appsmith JS Objects Issues related to JS Objects Query & JS Pod Issues related to the query & JS Pod Task A simple Todo

Comments

@sneha122
Copy link
Contributor

sneha122 commented Oct 28, 2024 •
edited
Loading

SubTasks

If we observe the Js object update call on new relic, for every existing action, we update in DB. During this process, on new relic we are observing uninstrumented time getting allocated when updating each action. This task is to add metrics around that uninstrumented time and also analyse the cause of it to see if it can be optimised.

Screenshot 2024-10-28 at 1 07 00 PM
@sneha122 sneha122 added the Task A simple Todo label Oct 28, 2024
@sneha122 sneha122 self-assigned this Oct 28, 2024
@sneha122 sneha122 added JS Objects Issues related to JS Objects Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. labels Oct 28, 2024
@github-actions github-actions bot added Javascript Product Issues related to users writing javascript in appsmith Query & JS Pod Issues related to the query & JS Pod Integrations Product Issues related to a specific integration labels Oct 28, 2024
@sneha122 sneha122 mentioned this issue Oct 29, 2024
Merged
2 tasks
@sneha122
Copy link
Contributor Author

sneha122 commented Nov 4, 2024

Moving this to sprint backlog in favor of a-force critical issue that has come up

github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this issue Nov 20, 2024
@sneha122
fix: skipped unnecessary code for js object updates (appsmithorg#37125)
300a3bd 
## Description

This PR is in continuation to
[PR](appsmithorg#37062) which we had
merged earlier, In previous, we skipped the redundant calls to update
page layout when updating each js object action, as we already have a
call for [updating the page layout for
actionCollection](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L411)

Since we have skipped the update page layout for each js action, we no
longer need the code part after this, which basically fetches page data
from DB and updates the `errorReports` in actionDTO based on layout
`layoutOnLoadActionErrors`. This PR skips this unnecessary part too for
each js action as we do [set the errorReport for actionCollection in the
end](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L430)

### Will this have any impact on error messages shown to user?
In order to understand this, checked out the frontend code to see if
errorReports from individual js action is getting consumed on updating
js object, looks like [it is
not](https://github.com/appsmithorg/appsmith/blob/e7e3d5e00290919c1df0767fdefed67458ec3cc9/app/client/src/sagas/JSPaneSagas.ts#L316),
so we can safely remove this piece of code.
However this points to existing bug in the code (as errorReports is not
even getting consumed from actionCollection), which is, when there is
cyclic dependency created for js object with a widget, we don't get any
toast message. Since this is existing issue which is not caused by any
of the above PR implementations, creating separate issue for it and
tracking it [here](appsmithorg#37129)

Fixes appsmithorg#37114 
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.JS, @tag.JS"

### 🔍 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/11698258739>
> Commit: 9fbde99
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11698258739&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.JS, @tag.JS`
> Spec:
> <hr>Wed, 06 Nov 2024 06:50:05 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

- **New Features**
- Streamlined layout update process for actions, enhancing performance
and clarity.

- **Bug Fixes**
- Improved test reliability by monitoring interactions with the layout
service and handling cyclic dependencies.

- **Documentation**
- Updated comments to clarify the logic behind layout updates for JS
actions.

- **Tests**
- Enhanced test descriptions and assertions for better clarity and
validation of method interactions, including cyclic dependency
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“sneha@appsmith.com”>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sneha122 sneha122

Labels
Integrations Pod General Issues related to the Integrations Pod that don't fit into other tags. Integrations Product Issues related to a specific integration Javascript Product Issues related to users writing javascript in appsmith JS Objects Issues related to JS Objects 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: skipped unnecessary code for js object updates appsmithorg/appsmith
1 participant
@sneha122