chore: Add spans attributes for no of lines and action count #37001
Conversation
github-actions
bot
added
skip-changelog
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...th-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java
Outdated
Show resolved
Hide resolved
...th-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java
Outdated
Show resolved
Hide resolved
...appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java
Outdated
Show resolved
Hide resolved
...th-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java
Outdated
Show resolved
Hide resolved
...appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java
Outdated
Show resolved
Hide resolved
|
@rishabhrathod01 can you please resolve merge conflicts and this comment, rest LGTM! |
rishabhrathod01
requested review from
nidhi-nair and
sondermanish
as code owners
WalkthroughA new Java class Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/LayoutSpanCE.java (1)
18-20: LGTM. Consider line breaks for readability.The new constants are well-defined and consistent with the existing pattern. For improved readability, consider breaking the longer constant definition into multiple lines:
public static final String EXTRACT_ALL_WIDGET_NAMES_AND_DYNAMIC_BINDINGS_FROM_DSL = APPSMITH_SPAN_PREFIX + "extractAllWidgetNamesAndDynamicBindingsFromDSL";app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (1)
378-388: Good addition of metrics, but consider refactoring for clarity.The new code adds valuable metrics for observability. However, consider extracting the calculations into separate methods for improved readability.
Consider refactoring like this:
-String body = actionCollectionDTO.getBody(); -Number lineCount = 0; -if (body != null && !body.isEmpty()) { - lineCount = body.split("\n").length; -} -Number actionCount = 0; -if (actionCollectionDTO.getActions() != null - && !actionCollectionDTO.getActions().isEmpty()) { - actionCount = actionCollectionDTO.getActions().size(); -} +Number lineCount = calculateLineCount(actionCollectionDTO.getBody()); +Number actionCount = calculateActionCount(actionCollectionDTO.getActions()); +private Number calculateLineCount(String body) { + return (body != null && !body.isEmpty()) ? body.split("\n").length : 0; +} +private Number calculateActionCount(List<ActionDTO> actions) { + return (actions != null && !actions.isEmpty()) ? actions.size() : 0; +}app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (1)
351-351: Remove Redundant Commented CodeThe commented line
// observationHelper.createSpan()appears unnecessary and can be removed for cleaner code.app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (1)
56-56: Correct the typo inEXECUTABLE_NAME_TO_EXECUTUTABLE_MAPThere's a typographical error in the constant name
EXECUTABLE_NAME_TO_EXECUTUTABLE_MAP. Please correct it toEXECUTABLE_NAME_TO_EXECUTABLE_MAPand update all references accordingly.Also applies to: 150-150
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/OnLoadSpan.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/LayoutSpanCE.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (11 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (13 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/OnLoadSpan.java
🧰 Additional context used
🔇 Additional comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceImpl.java (1)
4-4: LGTM. Observability enhancement looks good.The addition of
ObservationHelperImplaligns with the PR objectives for improved metrics tracking.Let's check if similar changes have been made in related classes:
Also applies to: 25-26, 35-36
✅ Verification successful
Observability enhancements are consistent across related classes. Verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar observability changes in related classes # Test: Search for classes with similar naming pattern rg -l 'class \w+ServiceImpl extends \w+ServiceCEImpl' | xargs rg -l 'ObservationHelperImpl' # Test: Check for constructor changes in these classes rg -l 'class \w+ServiceImpl extends \w+ServiceCEImpl' | xargs rg 'public \w+ServiceImpl\(.*ObservationHelperImpl.*\)'Length of output: 309
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java (5)
1-3: LGTM: Package declaration and import.The package declaration and import statement are correctly structured and follow Java conventions.
5-5: LGTM: Class declaration.The class name
OnLoadSpanCEis descriptive and follows Java naming conventions. TheCEsuffix is consistent with Appsmith's naming pattern for Community Edition classes.
7-21: LGTM: Constant definitions structure.The constant definitions follow Java conventions and consistently use
APPSMITH_SPAN_PREFIX. The multi-line formatting for longer values improves readability.
7-21: LGTM: Constant values.The constant values are consistently prefixed with
APPSMITH_SPAN_PREFIXand provide clear context for the operations they represent. TheEXECUTABLE_IN_CREATOR_CONTEXTvalue has been updated as requested in the previous review.
1-22: LGTM: Overall class structure and naming conventions.The
OnLoadSpanCEclass is well-structured, follows Java naming conventions, and adheres to the Single Responsibility Principle by focusing solely on defining span constants. The constants are consistently named and organized, making the code easy to read and maintain.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (2)
406-407: Metrics tagging looks good.The addition of
lineCountandactionCounttags enhances observability, aligning well with the PR objectives.
Line range hint
378-407: Overall, good implementation of metrics for observability.The changes effectively add metrics for line count and action count, enhancing the observability of the system. This aligns well with the PR objectives. Consider the refactoring suggestion for improved code clarity.
app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java (3)
20-20: Import 'ObservationHelperImpl' AddedThe import statement for
ObservationHelperImplis correctly added to support observability.
30-30: Import 'Span' from Micrometer TracingThe import of
Spanfromio.micrometer.tracingis appropriate for trace span management.
76-76: Added 'observationHelper' InstanceIntroducing the
observationHelperenhances observability within the service implementation.
...appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/OnLoadSpanCE.java
Outdated
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Show resolved
Hide resolved
...ver/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java
Show resolved
Hide resolved
….com/appsmithorg/appsmith into chore/add-JSObject-update-metrics-ce
…horg#37001) ## Description Fixes appsmithorg#36995 ## 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/11455841055> > Commit: 7285f3f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11455841055&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 22 Oct 2024 08:35:48 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added new constants for layout operations and data extraction to improve functionality. - Introduced observability tracking for various methods to enhance monitoring and performance insights. - **Bug Fixes** - Enhanced error handling and logging for action updates and layout modifications. - **Refactor** - Improved clarity and maintainability of methods related to action collections and layout updates. - **Chores** - Updated method signatures to include new observability parameters for better tracking. - Added metrics for line and action counts in action collection updates. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #36995
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/11455841055
Commit: 7285f3f
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 22 Oct 2024 08:35:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit