chore: added new relic telemetry for spans#34240
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThis update focuses on integrating performance metrics and logging to debug tenant fetching operations in the Appsmith application. By incorporating Micrometer's Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TenantService
participant ObservationRegistry
participant CacheableRepositoryHelper
participant Database
Client->>+TenantService: getDefaultTenant()
TenantService->>+ObservationRegistry: Observe Start
TenantService->>+CacheableRepositoryHelper: fetchDefaultTenant()
CacheableRepositoryHelper->>ObservationRegistry: Log Start
CacheableRepositoryHelper->>Database: Fetch Data
Database-->>CacheableRepositoryHelper: Return Data
CacheableRepositoryHelper->>ObservationRegistry: Log End
CacheableRepositoryHelper-->>TenantService: Return Data
TenantService->>ObservationRegistry: Observe End
TenantService-->>Client: Return Tenant
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
Integrations Product
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CacheableRepositoryHelperImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce_compatible/CacheableRepositoryHelperCECompatibleImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (5 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
User: NilanshBansal PR: appsmithorg/appsmith#33641 File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25 Timestamp: 2024-05-26T02:49:06.780Z Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CacheableRepositoryHelperImpl.java (1)
5-5: The integration ofObservationRegistryin the constructor is well-implemented for enhanced monitoring.Also applies to: 15-17
Verification successful
The integration of
ObservationRegistryin the constructor ofCacheableRepositoryHelperImpland its superclassCacheableRepositoryHelperCECompatibleImplis correctly implemented. The superclass is properly utilizing the passedObservationRegistry.
- Confirmed locations:
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/CacheableRepositoryHelperImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce_compatible/CacheableRepositoryHelperCECompatibleImpl.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the superclass correctly utilizes the passed `ObservationRegistry`. # Test: Search for the superclass usage of `ObservationRegistry`. Expect: Proper usage in superclass. rg --type java $'super.*observationRegistry' -C 3 src/main/java/com/appsmith/server/repositories/ce_compatible/CacheableRepositoryHelperCECompatibleImpl.javaLength of output: 306
Script:
#!/bin/bash # Locate all Java files that might contain the superclass definition. fd -e java # Search for the usage of `ObservationRegistry` across the located Java files. rg --type java 'super.*observationRegistry' -C 3Length of output: 190495
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (1)
17-17: The introduction ofObservationRegistryinCacheableRepositoryHelperCEImplenhances monitoring capabilities. Ensure that theObservationRegistryis used effectively in the methods where needed.Also applies to: 23-23, 41-49, 191-201
Verification successful
The
ObservationRegistryis properly integrated and utilized within theCacheableRepositoryHelperCEImplclass, specifically in thetapmethod withMicrometer.observation.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ObservationRegistry` is utilized effectively within `CacheableRepositoryHelperCEImpl`. # Test: Search for usage of `ObservationRegistry` within `CacheableRepositoryHelperCEImpl`. Expect: Effective usage. rg --type java $'observationRegistry' -C 3 src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.javaLength of output: 255
Script:
#!/bin/bash # Locate the correct path of CacheableRepositoryHelperCEImpl.java fd CacheableRepositoryHelperCEImpl.java # Assuming the correct path is found, verify the usage of `ObservationRegistry` within the file. # Replace <correct_path> with the actual path found. rg --type java $'observationRegistry' -C 3 <correct_path>Length of output: 356
Script:
#!/bin/bash # Verify the usage of `ObservationRegistry` within `CacheableRepositoryHelperCEImpl.java`. rg --type java $'observationRegistry' -C 3 app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.javaLength of output: 1199
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (4)
22-22: Great job on integratingObservationRegistryandMicrometerfor enhanced observability.Also applies to: 27-27
32-33: Usage of constants fromTenantSpanfor naming conventions in observability is a good practice as it maintains consistency and improves maintainability.
51-51: The injection ofObservationRegistrythrough the constructor ensures that the dependency is managed correctly. This is crucial for the observability of the tenant service operations.Also applies to: 62-62, 69-69
185-186: The application of observability to thegetDefaultTenantmethod usingname()andtap()from Micrometer is well-implemented. However, ensure that the observability is effectively capturing all necessary metrics and logs, particularly in error scenarios.Also applies to: 203-204
Verification successful
The
onErrorResumeblock in thegetDefaultTenantmethod includes logging for error scenarios, which is a good observability practice. The logs capture both the error and the subsequent action taken (evicting the cache and fetching from the database).
- Error logging:
log.error("Error fetching default tenant from redis!", e);- Action logging:
log.info("Evicting the default tenant from cache and fetching from the database!");These logs ensure that necessary metrics and logs are captured during error scenarios.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all necessary metrics and logs are captured in error scenarios. # Test: Search for error handling observability in the getDefaultTenant method. rg --type java 'onErrorResume' 'app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java'Length of output: 166
Script:
#!/bin/bash # Extract the onErrorResume block from getDefaultTenant method to inspect for observability measures. rg -A 10 'onErrorResume' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.javaLength of output: 1009
...om/appsmith/server/repositories/ce_compatible/CacheableRepositoryHelperCECompatibleImpl.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java
Show resolved
Hide resolved
...rver/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
Show resolved
Hide resolved
NilanshBansal
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/TenantSpan.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
2 participants
Description
Fixes #34072
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/9511509976
Commit: d4dc99e
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements