chore: Add an event queue in segment #39411
Conversation
WalkthroughThe changes adjust how user telemetry is handled by adding explicit calls to avoid analytics tracking when telemetry is disabled. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
IDE Pod
github-actions
bot
added
the
skip-changelog
![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 (1)
app/client/src/utils/Analytics/segment.ts (1)
110-118: Consider adding error handling in processEventQueue.While the queue processing logic is sound, it might benefit from error handling to prevent a single event failure from blocking the queue.
private processEventQueue() { while (this.eventQueue.length > 0) { const event = this.eventQueue.shift(); if (event) { - this.track(event.name, event.data); + try { + this.track(event.name, event.data); + } catch (error) { + log.error("Failed to process queued event:", error, event); + } } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/sagas/userSagas.tsx(1 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(2 hunks)app/client/src/utils/Analytics/segment.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (5)
app/client/src/utils/Analytics/segment.ts (3)
10-15: LGTM! Well-defined state enumeration.The
SegmentInitStateenum clearly defines all possible states of the Segment analytics system.
20-21: LGTM! Good state management implementation.The event queue and init state are properly encapsulated as private fields, following good OOP practices.
149-152: LGTM! Clean implementation of tracking avoidance.The
avoidTrackingmethod properly handles the state transition and queue cleanup.app/client/src/ce/utils/AnalyticsUtil.tsx (1)
134-138: LGTM! Clean implementation of analytics avoidance.The function properly initializes the segment instance and calls the avoidTracking method.
app/client/src/ce/sagas/userSagas.tsx (1)
250-251: LGTM! Proper handling of disabled telemetry.The saga correctly calls
avoidTrackingwhen telemetry is disabled, ensuring proper cleanup of queued events.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/utils/Analytics/segment.test.ts (1)
167-180: Consider adding test for queue behavior after initialization failure.The test verifies that initialization failure is properly logged, but doesn't validate what happens to queued events after initialization fails. According to the PR objectives, queued events should be logged locally if initialization fails.
it("should log events locally after initialization failure", async () => { mockAnalyticsBrowser.load.mockRejectedValueOnce(new Error("Init failed")); const segment = SegmentSingleton.getInstance(); const eventData = { test: "data" }; // Queue an event segment.track("test-event", eventData); // Initialization fails await segment.init(); // Queue another event after failed initialization segment.track("post-failure-event", { more: "data" }); // Analytics track shouldn't be called, but log should be called twice expect(mockAnalytics.track).not.toHaveBeenCalled(); expect(log.debug).toHaveBeenCalledTimes(2); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/utils/Analytics/segment.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (4)
app/client/src/utils/Analytics/segment.test.ts (4)
1-46: Well-structured test setup with comprehensive mocks.The test setup code is clear and follows best practices with proper mocking of external dependencies and resetting before each test.
48-56: Good validation of the singleton pattern.This test correctly verifies that multiple calls to getInstance() return the same instance.
58-100: Thorough initialization tests covering key scenarios.The tests appropriately verify behavior with valid API key, disabled segment, and fallback to ceKey when apiKey is empty.
102-138: Event queuing and processing tests validate core functionality.These tests validate the key functionality added in this PR - queuing events when not initialized and processing them after initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/client/src/utils/Analytics/segment.test.ts (4)
58-68: Consider adding a middleware verification test.The init test validates that load is called correctly, but doesn't verify that any source middleware is registered. Consider adding an assertion to check that
addSourceMiddlewarewas called.expect(mockAnalyticsBrowser.load).toHaveBeenCalledWith( { writeKey: "test-api-key" }, expect.any(Object), ); +expect(mockAnalytics.addSourceMiddleware).toHaveBeenCalled();
140-153: Test identify behavior when not initialized.The current test only checks identify behavior after initialization. Consider adding a test case for what happens when identify is called before initialization.
it("should queue identify calls when not initialized", async () => { const segment = SegmentSingleton.getInstance(); const userId = "test-user"; const traits = { name: "Test User" }; segment.identify(userId, traits); expect(mockAnalytics.identify).not.toHaveBeenCalled(); // Add appropriate expectation based on your implementation // (e.g., queuing, logging, etc.) });
155-165: Test reset behavior when not initialized.Add a test case for what happens when reset is called before initialization to ensure complete coverage of edge cases.
it("should handle reset when not initialized", () => { const segment = SegmentSingleton.getInstance(); segment.reset(); expect(mockAnalytics.reset).not.toHaveBeenCalled(); // Add appropriate expectation based on your implementation });
1-222: Add a test for when telemetry is disabled but initialization succeeds.According to the PR objectives, when telemetry is not enabled but initialization succeeds, the Segment should stop queueing events and log them locally. This scenario should be tested.
it("should log locally when telemetry is disabled but initialization succeeds", async () => { // Mock telemetry being disabled but segment still initialized (getAppsmithConfigs as jest.Mock).mockReturnValue({ segment: { enabled: true, // Segment enabled (for initialization) apiKey: "test-api-key", }, }); const segment = SegmentSingleton.getInstance(); await segment.init(); // Simulate telemetry being disabled after initialization segment.avoidTracking(); // Track an event segment.track("test-disabled-event", { test: "data" }); // Verify event is logged locally, not sent to analytics expect(mockAnalytics.track).not.toHaveBeenCalledWith("test-disabled-event", expect.any(Object)); expect(log.debug).toHaveBeenCalledWith( expect.stringContaining("Event logged locally"), "test-disabled-event", { test: "data" } ); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/utils/Analytics/segment.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/utils/Analytics/segment.test.ts (1)
181-221: The avoidTracking tests look good.These tests adequately verify the described behavior for the avoidTracking functionality, covering both the initialized and non-initialized states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/utils/Analytics/segment.ts (1)
110-118: Consider adding a queue size limit to prevent memory issues.The event queue has no size restrictions, which could lead to excessive memory usage if many events occur before initialization completes or fails.
- private eventQueue: Array<{ name: string; data: EventProperties }> = []; + private readonly MAX_QUEUE_SIZE = 100; + private eventQueue: Array<{ name: string; data: EventProperties }> = []; private processEventQueue() { while (this.eventQueue.length > 0) { const event = this.eventQueue.shift(); if (event) { this.track(event.name, event.data); } } }Then update the push in the track method:
if (this.initState === SegmentInitState.WAITING) { - this.eventQueue.push({ name: eventName, data: eventData }); + if (this.eventQueue.length < this.MAX_QUEUE_SIZE) { + this.eventQueue.push({ name: eventName, data: eventData }); + } else { + log.debug("Event queue full, dropping event", eventName); + } log.debug("Event queued for later processing", eventName, eventData); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/utils/Analytics/segment.test.ts(1 hunks)app/client/src/utils/Analytics/segment.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/utils/Analytics/segment.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (10)
app/client/src/utils/Analytics/segment.ts (10)
10-15: Good addition of state enum for tracking initialization states.The
SegmentInitStateenum provides a clear and type-safe way to track the different states of the segment analytics initialization process.
20-21: LGTM! Event queue implementation for handling initialization-time events.The event queue and initialization state tracking are well-implemented to cache events that occur during the initialization phase.
55-56: Appropriate early exit with avoidTracking when telemetry is disabled.This ensures that when segment is not enabled, we properly set the state to avoid tracking and clean up any queued events.
70-70: Good handling for missing write key scenario.Ensuring proper state handling when write key is not available.
95-97: Proper state management after successful initialization.The state is correctly updated to INITIALIZED and queued events are processed.
102-104: Appropriate error handling in the initialization flow.Good practice to flush the event queue and update the state on initialization failure.
125-129: Good conditional for queueing events only in WAITING state.This ensures events are only queued when the analytics system is still initializing.
131-135: Appropriate handling for when tracking is not required.This cleanly handles the case when telemetry is disabled, ensuring events are only logged locally.
153-156: Well-implemented avoidTracking method.The method cleanly handles the case when tracking should be avoided by setting the appropriate state and flushing the queue.
137-138: Good logging for fired events.Adding clear logging helps with debugging and monitoring analytics events.
Description
Adds an event queueing mechanism to cache events that have occurred during the init of segment.
This is being done to avoid loosing some early events that happen while we are still downloading the Analytics lib
Segment class will now be in waiting mode on start, where it will keep queuing events that have occurred.
When init is successful It will process the queue and get out of waiting state, processing further events as they occur/
When init has failed It will stop queuing and track further events by logging them locally.
When init is not required (telemetry not enabled) It will stop queuing and track further event by logging them locally.
Fixes #39026
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13517888023
Commit: 1429f4f
Cypress dashboard.
Tags: @tag.Sanity
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js
List of identified flaky tests.Tue, 25 Feb 2025 10:15:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests