[Fix] attributeEncoders are not registered nor called - #1441
Merged
Merged
Conversation
sbarrio
commented
Sep 22, 2026
| clearAllData: jest.fn().mockImplementation( | ||
| () => new Promise<void>(resolve => resolve()) | ||
| ) as jest.MockedFunction<DdNativeSdkType['clearAllData']>, | ||
| setAccountInfo: jest.fn().mockImplementation( |
Collaborator
Author
There was a problem hiding this comment.
These mocks were missing.
sbarrio
commented
Sep 22, 2026
| import { DdRumUserInteractionTracking } from '../../../rum/instrumentation/interactionTracking/DdRumUserInteractionTracking'; | ||
| import { BufferSingleton } from '../../../sdk/DatadogProvider/Buffer/BufferSingleton'; | ||
| import { NativeDdSdk } from '../../../sdk/DdSdkInternal'; | ||
| import NativeDdSdkSpec from '../../../specs/NativeDdSdk'; |
Collaborator
Author
There was a problem hiding this comment.
This fix has been applied across all tests. We should have always imported the native Spec from specs/NativeDdSdk but since DdSdkInternal wrongly exported it (instead of its own DdSdkWrapper) this import was technically correct, although very misleading.
This change makes it clear across the codebase.
sbarrio
force-pushed
the
sbarrio/fix/attribute-encoders-not-working
branch
from
September 22, 2026 12:22
a7dce45 to
bbfbcd1
Compare
sbarrio
commented
Sep 22, 2026
| }); | ||
| } | ||
| ); | ||
| describe('routing through DdSdkWrapper', () => { |
Collaborator
Author
There was a problem hiding this comment.
This new set of tests make sure that the DdSdkWrapper exported from DdsdkInternal is actually being called and not skipped.
🎉 All green!🧪 All tests passed 🔗 Commit SHA: bbfbcd1 | Docs | View more details | Give us feedback! |
marco-saia-datadog
approved these changes
Sep 22, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues remain around unsupported-value handling, native-module null safety, and custom encoder initialization coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Fixes SDK initialization so configured attributeEncoders are registered and used through DdSdkWrapper.
Changes:
- Routes SDK, RUM, telemetry, and buffer operations through the wrapper.
- Updates native spec references, tests, and React Native mocks.
- Adds wrapper-routing test coverage.
| File | Summary |
|---|---|
packages/core/src/sdk/EventMappers/EventMapper.ts |
Routes mapper telemetry through the wrapper. |
packages/core/src/sdk/EventMappers/__tests__/EventMapper.test.ts |
Updates mapper telemetry assertions. |
packages/core/src/sdk/DdSdkInternal.ts |
Registers encoders and delegates native calls; initialization coverage for custom encoders remains missing. |
packages/core/src/sdk/DatadogProvider/Buffer/BoundedBuffer.ts |
Routes buffered telemetry through the wrapper. |
packages/core/src/sdk/DatadogProvider/Buffer/__tests__/BoundedBuffer.test.ts |
Updates buffered telemetry assertions. |
packages/core/src/rum/instrumentation/interactionTracking/DdRumUserInteractionTracking.tsx |
Routes interaction telemetry through the wrapper; native-module null safety needs preservation. |
packages/core/src/rum/DdRum.ts |
Routes RUM telemetry through the wrapper. |
packages/core/src/rum/__tests__/DdRum.test.ts |
Updates RUM telemetry assertions. |
packages/core/src/DdSdkReactNative.tsx |
Routes initialization through the wrapper; unsupported values can still throw, and custom encoder initialization coverage is incomplete. |
packages/core/src/__tests__/rum/instrumentation/DdRumUserInteractionTracking.test.tsx |
Updates interaction tracking assertions. |
packages/core/src/__tests__/DdSdkReactNative.test.tsx |
Updates native assertions and adds wrapper-routing tests. |
packages/core/__mocks__/react-native.ts |
Adds required native SDK method mocks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+100
to
102
| await DdSdk.initialize( | ||
| DdSdkReactNative.buildConfiguration(configuration, params) | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #1426
attributeEncoders were not being properly registered nor called, even when set on the SDK configuration.
The main culprit of this was DdSdkInternal, which imported the Native SDK Spec and named it NativeDdSdk, used it on its own methods to properly wire calls from JS to the native SDK, but then, instead of exporting the DdSdkWrapper re-exported it as is, so when DdSdk.ts wrapped this export and exposed it as DdSdk it made it so calls to functions in DdSdk were directly being routed to the Native Spec, skipping the methods on DdSdkInternal altogether, including the attributeEncoders setup.
The fix is to properly import the spec as NativeDdSdkSpec, use it as such and then have DdSdkInternal export DdSdkWrapper, which is then wrapped as a singleton on DdSdk.ts.
This PR updates calls to NativeDdSdk as DdSdk across the board, makes it explicit when the NativeSpec is being called (on tests, mainly), adds missing mocks for unit tests and also adds a set of guarding tests to make sure that calls to DdSdk actually call the DdSdkInternal interface and then the native spec.
Motivation
Raised on #1426
Additional Notes
Tested locally on the example apps by adding a custom set of attributeEncoders:
Review checklist (to be filled by reviewers)