Pass InternalLogger through the SR mappers#3518
Conversation
3800e92 to
053d46e
Compare
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053d46e354
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private val internalLogger by lazy { | ||
| (Datadog.getInstance() as FeatureSdkCore).internalLogger | ||
| } |
There was a problem hiding this comment.
When Session Replay is enabled with an explicit/non-default sdkCore (or the configuration is built before the default SDK is initialized), this lazy property resolves Datadog.getInstance() independently of the internalLogger later passed into ComposeViewMapper.map(). Because the shared SemanticsUtils and RootSemanticsNodeMapper are constructed from this captured default/no-op logger, Compose telemetry/errors continue to be logged against the wrong core even though the mapper receives the correct feature logger at mapping time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
While possible, it requires restructuring how SessionReplayConfiguration and SessionReplay.enable() interact, moving logic between classes, and touching the public ExtensionSupport interface — which is out of scope for this PR. Can be a potential follow-up
| private val rootSemanticsNodeMapper = RootSemanticsNodeMapper(colorStringFormatter) | ||
|
|
||
| private val internalLogger by lazy { | ||
| (Datadog.getInstance() as FeatureSdkCore).internalLogger |
There was a problem hiding this comment.
Are you sure it is ok to use the default unnamed SDKCore here?
There was a problem hiding this comment.
it's not ideal but the alternative is to get the correct core through the constructor, which modifies the public api and means the customer can forget and it will silently break, or to make changes that are outside the scope of this pr
There was a problem hiding this comment.
That is all because different Utils classes require logger in the constructor and/or are initialized at the caller class creation.
If we move logger to the method or resolve Utils instances lazily when they are needed, that would allow to avoid this.
Would it be a big lift?
There was a problem hiding this comment.
You're right - I've now hooked the compose mappers up to the existing logger chain. It's more files touched in the pr but it's a better solution
8e7d906 to
564da32
Compare
564da32 to
c8b3549
Compare
d8e09d6
c8b3549 to
d8e09d6
Compare
5 participants
What does this PR do?
Pass internalLogger through the mappers. While this looks like a lot of changes it's mainly changing 2-3 lines in every class.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)