chore: Add metrics to newRelic for update JSobject Collection #35947
Conversation
WalkthroughThe recent changes enhance the Appsmith application by integrating observability features across several services and components. This involves adding an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionCollectionService
participant ObservationRegistry
User->>ActionCollectionService: Request to find Action Collection by ID
ActionCollectionService->>ObservationRegistry: Start observation
ActionCollectionService->>ActionCollectionRepository: Retrieve Action Collection
ActionCollectionRepository-->>ActionCollectionService: Return Action Collection
ActionCollectionService->>ObservationRegistry: Stop observation
ActionCollectionService-->>User: Return Action Collection
Assessment against linked issues
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
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
github-actions
bot
added
Javascript Product
rishabhrathod01
requested review from
nidhi-nair and
sondermanish
as code owners
rishabhrathod01
changed the title
github-actions
bot
added
Bug
![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.yaml
Review profile: CHILL
Files selected for processing (11)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ActionCollectionSpan.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (9 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutCollectionServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (7 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (5 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ActionCollectionSpan.java
Additional comments not posted (27)
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceImpl.java (1)
10-10: Great job integrating theObservationRegistry! 👍The constructor changes look good:
- The new
ObservationRegistryparameter is added correctly.- The superclass constructor call is updated to pass the
observationRegistry.These changes will allow the
UpdateLayoutServiceImplto utilize theObservationRegistryfor monitoring and tracking observability metrics.Also applies to: 23-24, 32-34
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceImpl.java (1)
26-27: Great job adding observability support! 👍The changes to the constructor look good:
- The new
ObservationRegistryparameter allows the class to support observability features.- The superclass constructor call is updated correctly to include the new parameter.
Keep up the excellent work in enhancing the application's monitoring capabilities! 🌟
Also applies to: 36-38
app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutActionServiceImpl.java (1)
28-30: Great work integrating the ObservationRegistry for enhanced monitoring! 👍The changes to the constructor of the
LayoutActionServiceImplclass look good:
The new
ObservationRegistryparameter is added to the constructor signature, allowing the class to have a dependency on an observation registry for monitoring or metrics collection purposes.The constructor body is updated to pass the
observationRegistryto the superclass constructor, ensuring that the observation registry is properly initialized and available for use in the superclass.These changes align with the provided summary, which mentions the addition of an
ObservationRegistryto several classes for better monitoring of actions and layout updates.Keep up the excellent work in enhancing the observability of the application! Let me know if you have any further questions.
Also applies to: 40-41
app/server/appsmith-server/src/main/java/com/appsmith/server/services/LayoutCollectionServiceImpl.java (1)
30-31: Great work on integrating observability features! 👍The changes to the constructor look good:
- The new
ObservationRegistryparameter is added correctly.- The superclass call is updated to include the new parameter.
These modifications enable the class to support observability features, which can help in monitoring and tracking the behavior of the
LayoutCollectionServiceImpl.Keep up the excellent work in enhancing the application's observability capabilities! 🌟
Also applies to: 42-43
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java (1)
1-40: Great work on defining the constants for action collection spans! 👍The code is well-structured, follows a clear naming convention, and adheres to best practices. The constants cover the essential operations related to action collections, making it easier to monitor and track the application's behavior.
A few key points:
- The constants are prefixed with
APPSMITH_SPAN_PREFIXto ensure they are not dropped or ignored, as defined inTracingConfig.java.- The constants are comprehensive and cover various aspects of action collections, such as creating, updating, and deleting actions, validating actions, updating layouts, and retrieving actions and pages.
- The comments provide clear explanations of the purpose and usage of the constants, enhancing the code's readability and maintainability.
Overall, the code is of high quality and ready to be merged. Keep up the excellent work!
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (3)
54-54: Looks good! The addition of theObservationRegistryfield is a positive change that enables the class to leverage Micrometer's observation capabilities for monitoring purposes. This will help in tracking and analyzing the performance of various actions within the application.
222-226: Great work! The addition of observability annotations in theupdateNewActionByBranchedIdmethod is a valuable enhancement. By leveraging theObservationRegistryand annotating thefindByIdandflatMapoperations with meaningful names and observations, you have enabled better tracking and monitoring of the action retrieval and update processes. This will provide valuable insights into the performance and behavior of these critical operations.
239-245: Excellent addition! The observability annotations in theupdateActionBasedOnContextTypemethod are a valuable enhancement. By annotating theupdateSingleActionandupdatePageLayoutsByPageIdmethod calls with meaningful names and observations using theObservationRegistry, you have enabled better tracking and monitoring of the action update and page layout update processes. This will provide valuable insights into the performance and behavior of these critical operations, helping in identifying potential bottlenecks and optimizing the application.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (5)
26-26: Good job adding the necessary import statements for observability!The new import statements
ObservationRegistryandMicrometerare required for integrating observability features using the Micrometer library.Also applies to: 30-30
41-41: Good job adding the import statement for action collection spans!The new import statement is required for using the action collection span constants defined in the
ActionCollectionSpanCEclass.
61-61: Good job adding theobservationRegistryfield!The new field is required for injecting the
ObservationRegistrydependency, which is used for observability tracking.
Line range hint
311-351: Excellent work integrating observability features in theupdateUnpublishedActionCollectionmethod!The changes enhance various asynchronous operations with observability capabilities using the
.name(...)and.tap(...)methods. This allows for better monitoring and performance tracking of these operations.Some key observations:
- The
createSingleActionandupdateNewActionByBranchedIdservice calls are enhanced with observability.- The
newValidActionIdsMonoanddeleteNonExistingActionMonoare enhanced with observability.- The
actionCollectionService.update,updateLayoutBasedOnContext,actionCollectionService.generateActionCollectionByViewMode,actionCollectionService.populateActionCollectionByViewMode, andactionCollectionService.saveLastEditInformationInParentcalls are enhanced with observability.These changes align with the goal of improving the traceability and performance analysis of the
updateUnpublishedActionCollectionmethod.Also applies to: 369-371, 390-391, 393-395, 401-402, 405-406, 409-410
Line range hint
1-461: Great job enhancing theLayoutCollectionServiceCEImplclass with observability features!The changes introduce observability tracking using the Micrometer library, which aligns with the goal of improving the traceability and performance analysis of the class.
The observability enhancements follow a consistent pattern using the
.name(...)and.tap(...)methods, making it easier to understand and maintain the code.Overall, the changes are well-structured and contribute to better monitoring and performance tracking of the
LayoutCollectionServiceCEImplclass.app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (6)
28-28: LGTM!The new import statements for
ObservationRegistryandMicrometerare necessary for integrating observability features. The code changes are approved.Also applies to: 35-35
51-51: LGTM!The new import statement for action collection constants is valid. The code changes are approved.
67-67: LGTM!The new
observationRegistryfield is properly declared and injected. The code changes are approved.
126-136: LGTM!The observability changes for the
extractAllWidgetNamesAndDynamicBindingsFromDSLmethod using Micrometer are implemented correctly. The code changes are approved.
198-199: LGTM!The observability changes for the
updateExecutablesExecuteOnLoad,findAndUpdateLayout, andupdateLayoutmethods using Micrometer are implemented correctly. The code changes are approved.Also applies to: 211-214, 241-242
299-300: LGTM!The observability changes for the
findPageByIdandupdateLayoutmethods within theupdatePageLayoutsByPageIdmethod using Micrometer are implemented correctly. The code changes are approved.Also applies to: 305-310
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (2)
90-91: Great work adding theObservationRegistrymock! 👍The
ObservationRegistrymock is correctly declared and will be injected into the services under test.
115-116: Excellent job updating thesetUpmethod! 🌟The changes in the
setUpmethod look great:
- The
observationRegistryis correctly passed to the service constructors, allowing the services to use it for observability.- Mocking the
observationConfig()method ensures that the tests can control the behavior of theObservationRegistry.Keep up the good work!
Also applies to: 128-129, 159-161
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (6)
23-23: Import statement looks good.The import statement for
ObservationRegistryis necessary to use it in the class.
32-32: Import statement looks good.The import statement for
Micrometeris necessary to use it in the class.
46-46: Import statement looks good.The import statement for
GET_ACTION_COLLECTION_BY_IDis necessary to use it in the class.
Line range hint
59-71: Constructor changes look good.The changes to the constructor are necessary to:
- Inject the
ObservationRegistrydependency via the constructor.- Make the
ObservationRegistryavailable for use in the class by assigning it to theobservationRegistryfield.Also applies to: 79-79
381-384:findByIdmethod changes look good.The changes to the
findByIdmethod are necessary to record metrics for thefindByIdoperation using theObservationRegistry.
Line range hint
59-71: Changes to the public entities look good.The changes to the public entities are necessary to:
- Inject the
ObservationRegistrydependency via the constructor.- Use the
ObservationRegistryto record metrics for thefindByIdoperation.Also applies to: 381-384
rishabhrathod01
changed the title
...nterfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Outdated
Show resolved
Hide resolved
...nterfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Outdated
Show resolved
Hide resolved
...nterfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Outdated
Show resolved
Hide resolved
...nterfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Outdated
Show resolved
Hide resolved
...nterfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Outdated
Show resolved
Hide resolved
...appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java
Outdated
Show resolved
Hide resolved
...th-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/UpdateLayoutSpanCE.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LayoutSpan.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/LayoutSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (15 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (6 hunks)
Files skipped from review due to trivial changes (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/LayoutSpan.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Additional comments not posted (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/LayoutSpanCE.java (2)
5-18: Great job defining theLayoutSpanCEclass to hold the span name constants! 👍The
LayoutSpanCEclass serves as a clean and organized container for the span name constants related to layout updates and on-load executables. Keeping these constants in a dedicated class improves code readability and maintainability.The naming convention for the constants is consistent and follows the
APPSMITH_SPAN_PREFIXpattern, which helps in identifying and categorizing the spans.Overall, the class is well-structured and fulfills its purpose effectively.
6-17: The span name constants are well-defined and follow a consistent naming convention. Great work! 🙌The constants in the
LayoutSpanCEclass are used to define span names for monitoring layout updates and on-load executables. The names are descriptive and clearly convey the purpose of each span.The constants follow a consistent naming convention, starting with the
APPSMITH_SPAN_PREFIXfollowed by a specific suffix related to the operation being monitored. This consistency makes it easy to understand and maintain the span names.The constants cover various aspects of layout updates, such as updating by page ID, method, DSL method, and context. Additionally, there are constants for on-load executables, including finding and updating them.
Having these constants centralized in the
LayoutSpanCEclass promotes code reusability and reduces the chances of typos or inconsistencies in span names across the codebase.Overall, the constants are well-defined and contribute to effective monitoring of layout updates and on-load executables.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (5)
26-26: Looks good! The new import statements are necessary for integrating observability features.The added import statements for
ObservationRegistry, Micrometer'sMicrometerclass, and span constants are required for enhancing the class with observability capabilities.Also applies to: 30-30, 41-45
65-65: Great! TheobservationRegistryfield is properly injected and will be used for observability.The new private final field
observationRegistryof typeObservationRegistryis correctly added to the class. It will be used to create and manage observations for tracing and monitoring purposes.
336-339: Excellent work integrating observability features in theupdateUnpublishedActionCollectionmethod!The
createSingleActionandupdateNewActionByBranchedIdmethod calls are enhanced with.name(...)and.tap(...)methods to provide span names and wrap the operations with observations using Micrometer. This allows for effective tracing and monitoring of these operations.Also applies to: 347-350
371-373: Great job adding observability to theupdateUnpublishedActionCollectionmethod!The
deleteNonExistingActionMonooperation,ACTION_COLLECTION_UPDATEoperation, andupdateLayoutBasedOnContextmethod call are enhanced with.name(...)and.tap(...)methods. This ensures that these operations are properly traced and monitored, providing better visibility into the action collection update process.Also applies to: 392-393, 395-397
403-404: Excellent work enhancing observability in theupdateUnpublishedActionCollectionmethod!The
generateActionCollectionByViewMode,populateActionCollectionByViewMode, andsaveLastEditInformationInParentmethod calls are enhanced with.name(...)and.tap(...)methods. This ensures that these operations are properly traced and monitored, providing better visibility into the action collection generation and population process.Also applies to: 407-408, 411-412
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
...r/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/PageSpanCE.java
Show resolved
Hide resolved
.../src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...mith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
|
Resolving the merge conflict with EE repo here https://github.com/appsmithorg/appsmith-ee/pull/5046 |
…thorg#35947) ## Description - Add newRelic spans to track performance of process involved in update JSObject collection Fixes appsmithorg#36049 ## Automation /test 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/10704346206> > Commit: 5bf63bb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10704346206&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Wed, 04 Sep 2024 15:29:57 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** - Introduced enhanced observability for action collections and layout updates, allowing better tracking and monitoring of actions within the application. - Added new constants related to action collections and layout updates for improved functionality and clarity. - **Bug Fixes** - Improved the reliability of action collection retrieval and layout updates through enhanced observability features. - **Tests** - Updated tests to incorporate observability metrics, ensuring better monitoring of actions during testing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #36049
Automation
/test js
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10704346206
Commit: 5bf63bb
Cypress dashboard.
Tags:
@tag.JSSpec:
Wed, 04 Sep 2024 15:29:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests