fix: fully honor block-anonymous-tracking flag on client and server#41973
fix: fully honor block-anonymous-tracking flag on client and server#41973salevine wants to merge 5 commits into
Conversation
The configure_block_event_tracking_for_anonymous_users flag left two gaps that let anonymous-user events through when it was on: - Client (shouldTrackUser): an active license short-circuited the flag, so licensed EE/cloud instances always tracked anonymous users. Removed the licenseActive bypass so the flag is honored on all instances. - Server (AnalyticsServiceCEImpl.sendEvent): the flag was only checked in sendObjectEvent. Direct sendEvent callers passing an anonymous userId bypassed it. Added a gate on the public sendEvent that blocks anonymous userIds before the x-anonymous-user-id is resolved, keeping the existing sendObjectEvent check for the session-user (signup) path. sendObjectEvent now routes through the extracted private sendEventInternal to avoid a redundant (Redis-backed) flag check on the hot anonymous path. usage-pulse billing/seat metering is intentionally out of scope. Adds unit tests: client shouldTrackUser matrix (incl. the license-bypass regression guard) and a server sendEvent anonymous-block guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
WalkthroughAnonymous-user analytics tracking now uses ChangesAnonymous User Analytics Gating
Usage Pulse Anonymous Identity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsServiceCEImpl
participant FeatureFlagService
participant SegmentAnalytics
Client->>AnalyticsServiceCEImpl: send anonymous analytics event
AnalyticsServiceCEImpl->>FeatureFlagService: check anonymous tracking flag
FeatureFlagService-->>AnalyticsServiceCEImpl: blocked, allowed, or error
alt blocked or unresolved
AnalyticsServiceCEImpl-->>Client: complete without sending
else allowed
AnalyticsServiceCEImpl->>SegmentAnalytics: enqueue event
end
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…S_USERNAME Code-review follow-ups on the anonymous-tracking gate: - sendEvent's anonymous gate now handles featureFlagService.check() errors with onErrorResume, failing closed (event dropped, chain completes) instead of propagating the error into previously fire-and-forget callers like GlobalExceptionHandler. Adds a regression test for the error path. - Align gate style with repo convention: raw if (isBlocked) and plain Mono.empty() (Reactor forbids null onNext, so the defensive forms bought nothing). - Client: use the existing ANONYMOUS_USERNAME constant from userConstants instead of the hardcoded "anonymousUser" literal in shouldTrackUser and its test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/ok-to-test tags="@tag.All" |
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AnalyticsServiceCEImplTest.java (2)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
ANONYMOUS_USERNAMEconstant instead of a hardcoded string.The PR objectives note that the client was updated to use
ANONYMOUS_USERNAMEinstead of a hardcoded"anonymousUser"string. The tests should reference the same server-side constant (if one exists) to avoid drift if the value ever changes.Run the following script to check for an existing constant:
#!/bin/bash # Description: Search for an ANONYMOUS_USERNAME or similar constant on the server side. rg -n 'ANONYMOUS_USERNAME|ANONYMOUS_USER' --type java app/server/appsmith-server/src/main/java/com/appsmith/server/Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AnalyticsServiceCEImplTest.java` at line 55, The test is using a hardcoded anonymous username string instead of the shared server constant, so update AnalyticsServiceCEImplTest to reference the existing ANONYMOUS_USERNAME symbol wherever sendEvent is called with the anonymous user. Check the server-side constant location first and use that value consistently in the test methods so the assertions stay aligned with the production contract.
41-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive test case for when the flag is disabled.
Both new tests cover the blocking paths (flag on, flag error). A third test where the flag resolves to
false(don't block) and asserting thatenqueueis called would confirm the gate works bidirectionally and strengthen confidence that the blocking tests aren't trivially passing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AnalyticsServiceCEImplTest.java` around lines 41 - 77, Add a positive regression test in AnalyticsServiceCEImplTest for the anonymous-user path when FeatureFlagService.check(configure_block_event_tracking_for_anonymous_users) resolves to false, so the gate does not short-circuit. Reuse AnalyticsServiceCEImpl with a mocked Analytics and FeatureFlagService, call sendEvent with an anonymous user, and verify the returned Mono completes and analytics.enqueue is invoked once. This should sit alongside the existing sendEvent_anonymousUserWithBlockFlagOn_doesNotEnqueueEvent and sendEvent_anonymousUserWhenFlagCheckErrors_completesWithoutEnqueueing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AnalyticsServiceCEImplTest.java`:
- Line 55: The test is using a hardcoded anonymous username string instead of
the shared server constant, so update AnalyticsServiceCEImplTest to reference
the existing ANONYMOUS_USERNAME symbol wherever sendEvent is called with the
anonymous user. Check the server-side constant location first and use that value
consistently in the test methods so the assertions stay aligned with the
production contract.
- Around line 41-77: Add a positive regression test in
AnalyticsServiceCEImplTest for the anonymous-user path when
FeatureFlagService.check(configure_block_event_tracking_for_anonymous_users)
resolves to false, so the gate does not short-circuit. Reuse
AnalyticsServiceCEImpl with a mocked Analytics and FeatureFlagService, call
sendEvent with an anonymous user, and verify the returned Mono completes and
analytics.enqueue is invoked once. This should sit alongside the existing
sendEvent_anonymousUserWithBlockFlagOn_doesNotEnqueueEvent and
sendEvent_anonymousUserWhenFlagCheckErrors_completesWithoutEnqueueing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc6fb33f-56ff-432f-8ded-d9f5dc444e0d
📒 Files selected for processing (4)
app/client/src/ce/sagas/userSagas.test.tsapp/client/src/ce/sagas/userSagas.tsxapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.javaapp/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/AnalyticsServiceCEImplTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/ce/sagas/userSagas.test.ts
- app/client/src/ce/sagas/userSagas.tsx
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java
salevine
added
ok-to-test
subrata71
left a comment
subrata71
left a comment
There was a problem hiding this comment.
This blocks Segment initialization for anonymous users when configure_block_event_tracking_for_anonymous_users is enabled, which looks correct for analytics tracking.
But usage-pulse still depends on AnalyticsUtil.getAnonymousId() when enableTelemetry && segment.enabled is true. In this new state, segmentAnalytics exists but has no Segment user, so getAnonymousId() can return undefined, and /api/v1/usage-pulse may stop sending anonymousUserId.
Can we also update usage-pulse to fall back to its local FALLBACK_KEY/nanoid() ID when AnalyticsUtil.getAnonymousId() is unavailable? That would preserve the PR’s stated scope that usage-pulse remains separate from Segment analytics blocking. Also could you please verify how does the PR behave in the EE repo?
…nymous users When configure_block_event_tracking_for_anonymous_users blocks Segment init, segmentAnalytics exists but has no user, so AnalyticsUtil.getAnonymousId() returns undefined. The telemetry-enabled anonymous branch of getUsagePulsePayload then sent no anonymousUserId, breaking anonymous usage counting. Extract the existing local-fallback-id logic into getOrCreateFallbackAnonymousId() and fall back to it whenever the Segment id is unavailable, keeping usage-pulse's identity independent of Segment analytics blocking. The helper also guards localStorage access so a private-mode/quota throw returns a per-call id instead of dropping the pulse. Adds unit coverage for getUsagePulsePayload across the telemetry on/off and Segment-available/unavailable branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hardening: guard Follow-up to the usage-pulse fallback change. Fix: wrap the read/write in a const getOrCreateFallbackAnonymousId = (): string => {
try {
let fallback = localStorage.getItem(FALLBACK_KEY);
if (!fallback) {
fallback = nanoid();
localStorage.setItem(FALLBACK_KEY, fallback);
}
return fallback;
} catch {
// localStorage unavailable (private mode, quota, disabled) — return a
// non-persisted per-call id so the pulse still carries an anonymousUserId.
return nanoid();
}
};Covered by a new unit test ( Verification: |
@subrata71 - I updated it to address this |
@salevine I don't see any recent changes which addresses this. Forgot to push maybe? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@salevine just checking in here. I still don’t see the usage-pulse fallback commit on this PR as mentioned above. |
Hacktron Security Check - SkippedReason: Billing required for Code Review seats
|
|
@subrata71 I re-checked this against the current PR head. Your usage-pulse concern was valid before the last push: when anonymous Segment tracking is blocked, The missing commit is now pushed on this PR: EE verification: I checked the EE client shape as well. There is no separate EE Local verification:
|
3 participants
Description
Makes the
configure_block_event_tracking_for_anonymous_usersfeature flag completely turn off anonymous-user event tracking. Two gaps let anonymous events through when the flag was on:Gap 1 — client license bypass (
app/client/src/ce/sagas/userSagas.tsx)shouldTrackUsershort-circuited on an active license:so licensed EE/cloud instances tracked anonymous users regardless of the flag. Removed the
licenseActivebypass — the flag is now honored on all instances.Gap 2 — ungated direct
sendEventpath (AnalyticsServiceCEImpl)The flag was only checked in
sendObjectEvent. Direct callers ofsendEvent(event, userId, props)with an anonymous userId bypassed it (and that path resolves the anonymous user to the clientx-anonymous-user-id). Added a gate on the publicsendEventthat blocks anonymous userIds before id resolution, and it fails closed if the flag state can't be resolved (drops the event, completes the chain). The existingsendObjectEventcheck is kept (it governs the session-user/signup path, wheresendEventreceives the new user's email rather than"anonymousUser").sendObjectEventnow routes through the extracted privatesendEventInternalto avoid a redundant (Redis-backed) flag check on the hot anonymous path (published-app page views / action executions).Scope
false, so existing instances are unchanged until an admin enables it./api/v1/usage-pulse) still emits the anonymous ID — that feeds billing/seat metering, not Segment analytics, and is treated separately.Tests
userSagas.test.tscovering theshouldTrackUsermatrix, including the regression guard that anonymous + telemetry-on + flag-on returnsfalseon a licensed instance.AnalyticsServiceCEImplTestcases: a direct anonymoussendEventenqueues nothing when the flag is on, and fails closed (no enqueue, no error) when the flag check errors.Ticket: #15380
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29952940414
Commit: c1cf491
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 22 Jul 2026 21:15:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests