chore: remove usage pulse error log for anonymous user #41413
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughAdded logging and softened validation in UsagePulseServiceCEImpl: invalid or blank anonymousUserId and edit-mode mismatches now log a warning and return Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UsagePulseService
participant Logger
Client->>UsagePulseService: createPulse(request)
alt viewMode == true
UsagePulseService->>UsagePulseService: proceed with normal pulse creation
UsagePulseService-->>Client: Mono<createdPulse>
else viewMode == false
UsagePulseService->>UsagePulseService: check anonymousUserId
alt anonymousUserId blank
UsagePulseService->>Logger: warn("anonymousUserId blank, ignoring")
UsagePulseService-->>Client: Mono.empty()
else anonymousUserId present
UsagePulseService->>UsagePulseService: set anonymous flags & associate user
UsagePulseService-->>Client: Mono<createdPulse>
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java (2)
52-54: Consider adding debug-level logging for observability.While silently ignoring these cases reduces log noise, having no trace makes it difficult to verify the behavior is working as intended or to detect if anonymous users are unexpectedly attempting edit-mode operations.
Consider adding a debug log statement:
} else if (FALSE.equals(usagePulseDTO.getViewMode()) && usagePulseDTO.getAnonymousUserId() != null) { // Anonymous users shouldn't hit edit mode pulses; ignore silently to avoid noisy logs. + log.debug("Ignoring edit-mode pulse for anonymous user: {}", usagePulseDTO.getAnonymousUserId()); return Mono.empty(); }
78-80: Consider adding debug-level logging for observability.Similar to the earlier change, having at least debug-level visibility into when anonymous pulses are ignored would aid in monitoring and troubleshooting without creating log noise in production.
Consider adding a debug log statement:
if (StringUtils.isBlank(usagePulseDTO.getAnonymousUserId())) { // Anonymous usage pulses without an identifier are ignored to avoid noisy logs. + log.debug("Ignoring anonymous usage pulse with blank identifier"); return Mono.empty(); }app/server/appsmith-server/src/test/java/com/appsmith/server/services/UsagePulseServiceTest.java (2)
136-142: Fix test naming inconsistency.This test method is missing the
test_prefix that other test methods in this class use (e.g., lines 46, 70, 82, 105).Apply this diff to maintain naming consistency:
-public void createPulse_inEditMode_withAnonymousUser_ignored() { +public void test_createPulse_inEditMode_withAnonymousUser_ignored() {
65-75: Consider adding test coverage for view-mode with blank anonymous user ID.There's a gap in test coverage: what happens when
viewMode=truebutanonymousUserIdis blank/null for an anonymous user? Based on the implementation, this should also be silently ignored (via lines 78-80), but there's no test verifying this scenario.Consider adding a test case:
@Test @WithUserDetails(value = "anonymousUser") public void test_AnonymousUserPulse_ViewMode_WithBlankId_Ignored() { UsagePulseDTO usagePulseDTO = new UsagePulseDTO(); usagePulseDTO.setViewMode(true); // anonymousUserId is not set, will be null/blank StepVerifier.create(usagePulseService.createPulse(usagePulseDTO)) .verifyComplete(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/UsagePulseServiceTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UsagePulseServiceTest.java (1)
66-75: Test correctly validates the silent-ignore behavior.The test properly verifies that anonymous pulses without an identifier in edit mode are now ignored rather than throwing an exception.
Failed server tests
|
abhvsn
left a comment
abhvsn
left a comment
There was a problem hiding this comment.
@subrata71 we added these logs to figure out the underlying issue where client may trigger errorneous calls to trigger usage pulse. As you can see we don't allow edit app access to anonymous user so we should never end up in this state. The fact that we are seeing these logs mean that client is at fault. Can we figure out the root cause instead of silently ignoring it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java (1)
83-85: Graceful handling of missing anonymous user id; consider log level if this is high-volumeUsing
StringUtils.isBlankavoids NPEs and treats empty/whitespace IDs correctly, and returningMono.empty()matches the “ignore bad pulses” behavior. If this condition is expected to be frequent in some deployments, you might consider downgrading toINFO/DEBUGlater to avoid accumulating many warnings in instance logs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-25T06:50:40.623Z
Learnt from: trishaanand
Repo: appsmithorg/appsmith PR: 38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.623Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UsagePulseServiceCEImpl.java (2)
18-29: Appropriate use of@Slf4jfor lightweight loggingImporting
lombok.extern.slf4j.Slf4jand annotating the service keeps logging boilerplate-free and is consistent with the newlog.warnusage below. No issues here.
55-59: Soft-failing edit-mode pulses from anonymous users looks consistent with the PR goalSwitching from throwing to logging a concise warning and returning
Mono.empty()forviewMode=false+anonymousUserIdpresent removes noisy stack traces while still surfacing intent in logs. Just make sure all call sites treat a completed emptyMonoas “best-effort/no-op” and don’t rely on errors here for control flow.
Failed server tests
|
2 participants
Description
When customer shares logs from their instance it becomes really cumbersome to troubleshoot and figure out the main problem. The log file gets cluttered by the following error log:
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19664146229
Commit: 5394bfa
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 25 Nov 2025 09:44:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.