chore: Add JSObject creation flow metrics#37064
Conversation
WalkthroughThe pull request introduces new constants to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
skip-changelog
rishabhrathod01
requested review from
nidhi-nair and
sondermanish
as code owners
![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 (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/DatasourceSpanCE.java (1)
10-10: LGTM! Consider adding a brief comment.The new span constant follows the established naming pattern and aligns well with the monitoring objectives.
Consider adding a brief Javadoc comment to describe the specific operation this span measures:
+ /** Span for measuring datasource lookup by ID operation */ public static final String FIND_DATASOURCE_BY_ID = APPSMITH_SPAN_PREFIX + "datasourceService.findById";app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
378-381: Consider adding explicit error handling for datasource lookup.While the span is correctly placed, consider adding explicit error handling to better track datasource lookup failures in the metrics.
return datasourceService .findById(savedActionDTO.getDatasource().getId()) .name(FIND_DATASOURCE_BY_ID) - .tap(Micrometer.observation(observationRegistry)); + .tap(Micrometer.observation(observationRegistry)) + .onErrorResume(error -> { + log.error("Error finding datasource: {}", error.getMessage()); + return Mono.empty(); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/DatasourceSpanCE.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/PageSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (5 hunks)
🔇 Additional comments (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/PageSpanCE.java (2)
17-18: LGTM! Clear and consistent span naming.The updated span name better reflects the actual method being monitored, making it easier to correlate traces with code.
21-22: LGTM! Well-documented span addition.The new span follows the established naming pattern and is properly documented with its purpose.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionSpanCE.java (1)
42-46: LGTM! The new span constants follow conventions and support monitoring objectives.The added constants properly follow the required "appsmith." prefix convention and clearly describe their respective monitoring points in the JS action creation flow.
Let's verify these constants are being used in the action creation flow:
✅ Verification successful
Let me verify the usage of these span constants in the monitoring context.
The span constants are correctly implemented and used for monitoring
The constants are properly used with Micrometer observations in the action creation flow:
VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXTspans the domain validation inLayoutActionServiceCEImplVALIDATE_AND_SAVE_ACTION_TO_REPOSITORYspans the repository save operation across service implementations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new span constants in the codebase rg "validateAndGenerateActionDomainBasedOnContext|validateAndSaveActionToRepository" \ --type java \ -g '!**/ActionSpanCE.java'Length of output: 2416
Script:
#!/bin/bash # Search for span/tracing usage around these methods rg -B 2 -A 2 "validateAndGenerateActionDomainBasedOnContext|validateAndSaveActionToRepository" \ --type java \ -g '!**/ActionSpanCE.java' \ -g '!**/test/**'Length of output: 5592
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (4)
39-44: LGTM: Span imports are properly organized.The new span constants are correctly imported and utilized throughout the code.
345-346: LGTM: Proper span placement for action validation.The observation span correctly wraps the action validation logic.
412-413: LGTM: Proper span placement and error handling for page lookup.The observation span is correctly placed with appropriate error handling.
425-428: LGTM: Well-structured name validation with proper error handling.The observation span is correctly placed with appropriate error handling and user-friendly messages.
rishabhrathod01
requested review from
sneha122
and removed request for
nidhi-nair and
sondermanish
| public static final String FILL_SELF_REFERENCING_PATHS_ACTION = | ||
| APPSMITH_SPAN_PREFIX + "action.fillSelfReferencingPaths"; | ||
|
|
||
| public static final String VALIDATE_AND_GENERATE_ACTION_DOMAIN_BASED_ON_CONTEXT = |
There was a problem hiding this comment.
@rishabhrathod01 can we please follow the nomenclature for spans which uses the namespace of the logical product area that a span is added for? For example, consolidated spans are under appsmith.consolidated-api , git spans are under appsmith.git
There was a problem hiding this comment.
@nidhi-nair There was a concern about span name truncation after 50 chars on newRelic due to which it becomes difficult to filter them. Initially, I followed the practice of appsmith.domain.class.methodName or appsmith.domain.methodName for few span but that exceeds the char limit.
I will update the changes to follow the nomenclature by limiting the domain name char.
Failed server tests
|
|
|
||
| // datasource service spans | ||
| public static final String DATASOURCE_SERVICE = "datasourceService"; | ||
| public static final String FIND_DATASOURCE_BY_ID = APPSMITH_SPAN_PREFIX + DATASOURCE_SERVICE + ".findById"; |
There was a problem hiding this comment.
@rishabhrathod01 Should we use domain name here instead of datasourceService? I see you have already declared the domain of datasources
There was a problem hiding this comment.
@sneha122 I intended to create datasourceService as a domain name here to ease finding the method.
|
@rishabhrathod01 Changes LGTM |
## Description These changes add monitoring spans to analyze the JS action creation flow. Fixes appsmithorg#37065 ## Test the changes | Case | Screenshot | |---|---| | newRelic metrics for create flow |  | ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 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/11500044480> > Commit: ebf826f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11500044480&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Thu, 24 Oct 2024 13:46:02 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** - Introduced new constants to enhance action validation and repository operations. - Added functionality for improved observability in action creation and validation processes. - New constants related to data sources and pages for better tracking and logging. - **Bug Fixes** - Updated existing constants for better alignment with method names, enhancing clarity and functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
These changes add monitoring spans to analyze the JS action creation flow.
Fixes #37065
Test the changes
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11500044480
Commit: ebf826f
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 24 Oct 2024 13:46:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes