feat: add dmt support for cloned destinations#2434
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds Device Mode Transformation (DMT) support for cloned destinations by enabling DMT to work with destination clones that share the same original configuration.
- Removes the restriction that prevented cloned destinations from applying device mode transformations
- Implements proper handling of cloned destinations by using
originalIdfor transformation requests while preserving individual clone identities - Adds deduplication logic to prevent duplicate log messages when multiple clones of the same destination fail transformation
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts |
Removes cloned destination exclusion from transformation eligibility check |
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts |
Adds helper functions for destination ID handling and log deduplication, updates transformation logic to work with cloned destinations |
packages/analytics-js-plugins/src/deviceModeTransformation/index.ts |
Updates destination ID collection to use originalId and prevent duplicates |
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts |
Sets originalId property when creating cloned destinations |
packages/analytics-js-common/src/types/Destination.ts |
Adds optional originalId property to Destination type |
| Test files | Comprehensive test coverage for cloned destination functionality and log deduplication |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2434 +/- ##
============================================
+ Coverage 65.69% 91.40% +25.70%
============================================
Files 483 207 -276
Lines 16673 5979 -10694
Branches 3358 1167 -2191
============================================
- Hits 10954 5465 -5489
+ Misses 4544 488 -4056
+ Partials 1175 26 -1149 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
participant Source
participant DMT as DeviceModeTransformation.enqueue
participant Q as Queue
participant Svc as sendTransformedEventToDestinations
participant Dst as Destinations
Source->>DMT: enqueue(event, destinations)
DMT->>DMT: Build unique destinationIds using getDestinationId(dest) (originalId ?? id)
DMT->>Q: addItem({ destinationIds, event })
Q-->>DMT: ack
Q->>Svc: process(event, destinationIds)
Svc->>Svc: For each dest: destinationId = getDestinationId(dest)
Svc->>Dst: route transformed/untransformed per destinationId
Svc->>Svc: logOncePerDestination(destinationId, ...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (1)
260-267: Preserve originalId chain when cloning destinationsTo ensure that cloning an already-cloned destination retains the very first original ID, update the assignment in
applySourceConfigurationOverrides:• File: packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
Replace on line 265:- overriddenDestination.originalId = dest.id; + // Keep the full ancestry: if dest is already a clone, preserve its originalId + overriddenDestination.originalId = dest.originalId ?? dest.id;• Add a unit test for the “clone-of-clone” case:
- Start with a destination that already has
originalIdset- Apply two overrides (to force two levels of cloning)
- Assert that the final clone’s
originalIdequals the very first source ID, not the intermediate one
🧹 Nitpick comments (2)
packages/analytics-js-common/src/types/Destination.ts (1)
147-152: Document the semantics of originalId on cloned destinationsAdd a short TSDoc to clarify that originalId is set only on clones and always points to the first/original destination’s id. This avoids misuse down the line and makes intent explicit.
export type Destination = { id: string; + /** + * If present, identifies the original destination id for cloned instances. + * Populated only on clones; remains undefined for originals. + */ originalId?: string; displayName: string;packages/analytics-js-plugins/src/deviceModeTransformation/index.ts (1)
104-110: Deduplicate destinationIds using getDestinationId and Set to ensure consistency and O(n) performance
- Use the shared utility getDestinationId to centralize id resolution logic (originalId-aware).
- Use a Set to avoid O(n^2) includes checks while preserving insertion order.
Please confirm that the intended behavior is to group originals and their clones under a single destinationId (originalId), resulting in one transformation per original group. If originals and clones should be distinct, the ID resolution logic should be adjusted accordingly.
- const destinationIds: string[] = []; - destinations.forEach(d => { - const id = d.originalId ?? d.id; - if (!destinationIds.includes(id)) { - destinationIds.push(id); - } - }); + const destinationIds = Array.from( + new Set(destinations.map(d => getDestinationId(d))), + );Outside this hunk, update the utilities import:
// at the existing import from './utilities' import { createPayload, sendTransformedEventToDestinations, getDestinationId } from './utilities';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/analytics-js-common/src/types/Destination.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts(3 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts(1 hunks)packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts(2 hunks)packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/index.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(5 hunks)packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/002-javascript-typescript.mdc)
Code must work in browsers AND service workers (no Node.js APIs)
Files:
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.tspackages/analytics-js-plugins/src/deviceModeTransformation/index.tspackages/analytics-js-common/src/types/Destination.tspackages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.tspackages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.tspackages/analytics-js-plugins/src/deviceModeDestinations/utils.tspackages/analytics-js-plugins/src/deviceModeTransformation/utilities.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts
🧠 Learnings (10)
📚 Learning: 2024-10-08T15:52:59.819Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-10-08T15:52:59.819Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
Applied to files:
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.tspackages/analytics-js-plugins/src/deviceModeTransformation/index.tspackages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.tspackages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.tspackages/analytics-js-plugins/src/deviceModeDestinations/utils.tspackages/analytics-js-plugins/src/deviceModeTransformation/utilities.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts
📚 Learning: 2024-10-28T08:03:12.163Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
Applied to files:
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.tspackages/analytics-js-plugins/src/deviceModeTransformation/index.tspackages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.tspackages/analytics-js-plugins/src/deviceModeTransformation/utilities.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts
📚 Learning: 2024-10-28T08:19:43.438Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js/src/components/core/Analytics.ts:125-127
Timestamp: 2024-10-28T08:19:43.438Z
Learning: In `packages/analytics-js/src/components/core/Analytics.ts`, inputs to the `clone` function are sanitized prior, so error handling for clone operations is not needed.
Applied to files:
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts
📚 Learning: 2024-11-06T16:32:46.257Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-plugins/src/xhrQueue/index.ts:34-34
Timestamp: 2024-11-06T16:32:46.257Z
Learning: The deprecated plugins have been removed from the `PluginName` type, so the explicit `PluginName` type annotation is no longer necessary in `packages/analytics-js-plugins/src/xhrQueue/index.ts`.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/index.ts
📚 Learning: 2024-11-08T13:17:51.356Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/utilities/retryQueue/utilities.ts:0-0
Timestamp: 2024-11-08T13:17:51.356Z
Learning: The issue regarding missing test coverage for the `findOtherQueues` function in `packages/analytics-js-common/src/utilities/retryQueue/utilities.ts` is no longer applicable.
Applied to files:
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.tspackages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts
📚 Learning: 2024-07-27T07:02:57.329Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js/__tests__/nativeSdkLoader.js:31-33
Timestamp: 2024-07-27T07:02:57.329Z
Learning: The loading snippet in `packages/analytics-js/__tests__/nativeSdkLoader.js` is a standard part of the SDK, and no changes are desired on it.
Applied to files:
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts
📚 Learning: 2024-10-07T05:43:26.038Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-integrations/__tests__/integrations/Amplitude/browser.test.js:193-194
Timestamp: 2024-10-07T05:43:26.038Z
Learning: In the Amplitude integration tests (`browser.test.js`), the tests no longer rely on `appVersion`, so including `appVersion` in the mocked navigator object is unnecessary.
Applied to files:
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts
📚 Learning: 2024-11-09T06:40:30.520Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1823
File: packages/analytics-js-common/src/types/LoadOptions.ts:0-0
Timestamp: 2024-11-09T06:40:30.520Z
Learning: In the `packages/analytics-js-common/src/types/LoadOptions.ts` file, the `dataplanes` property within the `SourceConfigResponse` type has been removed as it is no longer necessary.
Applied to files:
packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts
📚 Learning: 2024-10-28T08:08:55.995Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:25-26
Timestamp: 2024-10-28T08:08:55.995Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the `getDeliveryPayload` function does not require additional error handling for stringification failures.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
📚 Learning: 2024-10-28T08:03:57.623Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:57.623Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the utility function `getDeliveryPayload` cannot fail because all problematic fields are cleaned up prior to this step.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
🔇 Additional comments (17)
packages/analytics-js-plugins/src/nativeDestinationQueue/utilities.ts (1)
99-101: LGTM: transformation eligibility simplifiedReturning dest.shouldApplyDeviceModeTransformation aligns the queue logic with clone-aware behavior elsewhere.
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (3)
584-587: LGTM: verifies originalId propagation on dest1 clonesThese assertions correctly validate that both clones carry originalId pointing to the original destination id.
612-612: LGTM: verifies originalId propagation on dest2 cloneValidates originalId on the surviving clone (after disabled clone is filtered out).
653-656: LGTM: verifies originalId propagation on dest3 clonesBoth clones correctly reference the original’s id.
packages/analytics-js-plugins/__tests__/nativeDestinationQueue/utilities.test.ts (2)
135-140: Good test coverage for cloned destinations.The test appropriately validates that the simplified logic returns
trueregardless of theclonedproperty value whenshouldApplyDeviceModeTransformationis true. This ensures consistent transformation behavior for both original and cloned destinations.
126-129: Function name and behavior align correctlyThe
shouldApplyTransformationfunction simply returnsdest.shouldApplyDeviceModeTransformation, which matches its name and the test description. The existing test accurately verifies that behavior, so no changes are needed.packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts (1)
374-378: LGTM! Proper deduplication of destination IDs for cloned destinations.The test correctly validates that when multiple cloned destinations share the same
originalId, only one unique ID is included in thedestinationIdsarray. This ensures efficient transformation requests without duplicates.packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts (6)
50-53: LGTM! Clean implementation of destination ID resolution.The helper function correctly implements the fallback logic for obtaining the destination identifier, prioritizing
originalIdwhen available.
55-67: LGTM! Effective deduplication mechanism for logging.The helper function efficiently prevents duplicate log messages for the same destination by maintaining a tracking array. This is particularly useful when processing multiple cloned destinations that share the same
originalId.
83-84: LGTM! Correct filtering logic for cloned destinations.The filter now properly uses
d.originalId ?? d.idto match destinations, which ensures that cloned destinations are correctly identified by their original ID when processing transformation responses.
86-91: LGTM! Well-structured deduplication implementation.The introduction of
loggedDestinationstracking array and consistent use ofgetDestinationIdensures that each unique destination (based on originalId) only generates one log message, even when multiple clones are processed.
110-119: LGTM! Consistent logging deduplication across all paths.The logging for transformation failures is properly wrapped with
logOncePerDestination, ensuring that error and warning messages are deduplicated across cloned destinations while maintaining the appropriate log level based on thepropagateEventsUntransformedOnErrorsetting.Also applies to: 121-130
195-200: LGTM! Appropriate exports for the new utility functions.The new helper functions
logOncePerDestinationandgetDestinationIdare correctly exported, making them available for testing and potential use in other modules.packages/analytics-js-plugins/__tests__/deviceModeTransformation/utilities.test.ts (4)
463-785: Comprehensive test coverage for cloned destinations.Excellent test suite that thoroughly covers various scenarios for cloned destinations including:
- Multiple clones with the same originalId
- Different propagation settings across clones
- Mixed original and cloned destinations
- Clones without their original destination in state
The tests properly validate that the transformation logic works correctly with the new originalId-based identification.
787-991: Excellent test coverage for log deduplication.The test suite comprehensively validates the log deduplication behavior across multiple cloned destinations. It correctly verifies that:
- Error logs appear only once for multiple clones with the same originalId
- Warning logs are deduplicated for server access errors
- Mixed propagation settings are handled correctly
- The deduplication is based on the destinationId (originalId) rather than the clone's individual ID
994-1101: Thorough testing of the logOncePerDestination utility.The test suite provides excellent coverage for the deduplication helper function, including edge cases like:
- Empty string destination IDs
- Complex destination IDs with special characters
- Pre-populated logged destinations arrays
- Isolation between different logging contexts
1103-1185: Complete test coverage for getDestinationId utility.The tests properly validate all scenarios for the destination ID resolution logic, including when originalId is present, absent, null, or undefined. The fallback behavior to the destination's id is correctly tested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts (1)
15-19: Polish: fix punctuation and handle undefined status more cleanlyMinor readability/log-quality nits:
- There's a stray period before “Transformation” resulting in
].Transformation. Prefer a space after the closing bracket.- When status is undefined, logging “undefined” is noisy. Consider a human-friendly fallback like “unknown”.
- Optional: make the status parameter optional (
status?: number) instead ofnumber | undefinedfor idiomatic TS.Proposed diff:
-const DMT_REQUEST_FAILED_ERROR = ( +const DMT_REQUEST_FAILED_ERROR = ( context: string, displayName: string, id: string, - status: number | undefined, + status?: number, action: string, ): string => - `${context}${LOG_CONTEXT_SEPARATOR}[Destination: ${displayName} with id "${id}"].Transformation request failed with status: ${status}. Retries exhausted. ${action}.`; + `${context}${LOG_CONTEXT_SEPARATOR}[Destination: ${displayName} with id "${id}"] Transformation request failed with status: ${status ?? 'unknown'}. Retries exhausted. ${action}.`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/analytics-js-plugins/src/deviceModeTransformation/index.ts(2 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts(1 hunks)packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-plugins/src/deviceModeTransformation/index.ts
- packages/analytics-js-plugins/src/deviceModeTransformation/utilities.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/002-javascript-typescript.mdc)
Code must work in browsers AND service workers (no Node.js APIs)
Files:
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
🧠 Learnings (3)
📚 Learning: 2024-10-28T08:03:12.163Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:0-0
Timestamp: 2024-10-28T08:03:12.163Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the issue regarding inconsistent error handling approaches in `getDMTDeliveryPayload` is no longer valid.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
📚 Learning: 2024-10-08T15:52:59.819Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11
Timestamp: 2024-10-08T15:52:59.819Z
Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
📚 Learning: 2024-10-28T08:08:55.995Z
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-plugins/src/utilities/eventsDelivery.ts:25-26
Timestamp: 2024-10-28T08:08:55.995Z
Learning: In `packages/analytics-js-plugins/src/utilities/eventsDelivery.ts`, the `getDeliveryPayload` function does not require additional error handling for stringification failures.
Applied to files:
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts
🧬 Code Graph Analysis (1)
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts (4)
packages/analytics-js-integrations/src/integrations/TVSquared/utils.js (1)
action(23-28)packages/analytics-js-common/src/constants/logMessages.ts (1)
LOG_CONTEXT_SEPARATOR(25-25)packages/analytics-js-plugins/src/shared-chunks/common.ts (1)
LOG_CONTEXT_SEPARATOR(31-31)scripts/update-integration-name-constants-references.js (1)
displayName(55-55)
⏰ 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: Code quality checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (3)
packages/analytics-js-plugins/src/deviceModeTransformation/logMessages.ts (3)
6-10: LGTM: destination id inclusion aligns with per-destination loggingAdding the id parameter and weaving it into the message is consistent with the cloned-destination DMT strategy and helps dedupe logs per destination. No issues spotted here.
21-22: Verify intent: DMT_EXCEPTION omits context while others include itThis helper doesn’t take a context or prepend LOG_CONTEXT_SEPARATOR, unlike the others. If that’s intentional (e.g., used in places where context is already provided upstream), all good. If not, consider aligning its signature and formatting with the others for consistency.
3-22: Follow-up: ensure all call sites now pass idThree helpers now require an id parameter. Please double-check callers have been updated across deviceModeTransformation/utilities.ts and index.ts (and any other usage) to avoid type or runtime issues, especially in tests.
|



PR Description
Add DMT support for cloned destinations
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests