Skip to content

REPLAY-1895 use internalLogger in SR modules#1574

Merged
mariusc83 merged 1 commit into
developfrom
mconstantin/replay-1364/use-internal-logger-in-sr-modules
Aug 16, 2023
Merged

REPLAY-1895 use internalLogger in SR modules#1574
mariusc83 merged 1 commit into
developfrom
mconstantin/replay-1364/use-internal-logger-in-sr-modules

Conversation

@mariusc83

@mariusc83 mariusc83 commented Aug 9, 2023

Copy link
Copy Markdown
Member

What does this PR do?

A brief description of the change being made with this pull request.

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)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@mariusc83 mariusc83 self-assigned this Aug 9, 2023
@mariusc83
mariusc83 force-pushed the mconstantin/replay-1364/use-internal-logger-in-sr-modules branch 3 times, most recently from eb8df90 to 5a49661 Compare August 10, 2023 08:01
@mariusc83
mariusc83 marked this pull request as ready for review August 10, 2023 08:01
@mariusc83
mariusc83 requested a review from a team as a code owner August 10, 2023 08:01
plousada
plousada previously approved these changes Aug 10, 2023
View reviewed changes
Comment thread ...rc/main/kotlin/com/datadog/android/sessionreplay/internal/processor/RecordedDataProcessor.kt
val wireframes = snapshots.flatMap { nodeFlattener.flattenNode(it) }

if (wireframes.isEmpty()) {
// TODO: RUMM-2397 Add the proper logs here once the sdkLogger will be added

@plousada plousada Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure we don't want to log anything here if there are no wireframes.

@mariusc83 mariusc83 Aug 11, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we do not as it might happen and it is normal...and also there is no actionable thing I can think of right now

Comment thread ...rc/main/kotlin/com/datadog/android/sessionreplay/internal/processor/RumContextDataHandler.kt Outdated
}

companion object {
const val INVALID_RUM_CONTEXT_ERROR_MESSAGE = "SR RumContextDataHandler: Invalid RUM " +

@plousada plousada Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add the context in question to better understand what is not present and make the telemetry more actionable?

Comment thread ...eplay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/WindowInspector.kt

@Suppress("SwallowedException", "TooGenericExceptionCaught", "UnsafeThirdPartyFunctionCall")
private val GLOBAL_WM_CLASS by lazy {
try {

@plousada plousada Aug 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the try catches here?

@mariusc83 mariusc83 Aug 11, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because they are all handled in that method there getGlobalViews

xgouchet
xgouchet suggested changes Aug 10, 2023
View reviewed changes
Comment thread ...id-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/gson/GsonExt.kt Outdated
Comment thread ...id-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/gson/GsonExt.kt Outdated
Comment thread ...id-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/gson/GsonExt.kt
Comment thread ...id-session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/gson/GsonExt.kt
Comment thread ...ay/src/main/kotlin/com/datadog/android/sessionreplay/internal/net/BatchesToSegmentsMapper.kt
Comment thread ...eplay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/WindowInspector.kt Outdated
Comment thread ...eplay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/WindowInspector.kt
Comment thread ...otlin/com/datadog/android/sessionreplay/internal/recorder/callback/RecorderWindowCallback.kt Outdated
Comment thread ...otlin/com/datadog/android/sessionreplay/internal/recorder/callback/RecorderWindowCallback.kt Outdated
Comment thread ...otlin/com/datadog/android/sessionreplay/internal/recorder/callback/RecorderWindowCallback.kt Outdated
@codecov-commenter

codecov-commenter commented Aug 10, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1574 (5a49661) into develop (0d85e50) will increase coverage by 0.06%.
Report is 2 commits behind head on develop.
The diff coverage is 79.08%.

@@             Coverage Diff             @@
##           develop    #1574      +/-   ##
===========================================
+ Coverage    83.27%   83.33%   +0.06%     
===========================================
  Files          437      437              
  Lines        14763    14874     +111     
  Branches      2223     2225       +2     
===========================================
+ Hits         12293    12394     +101     
- Misses        1881     1888       +7     
- Partials       589      592       +3
Files Changed Coverage Δ
...sionreplay/internal/net/BatchesToSegmentsMapper.kt 74.23% <43.75%> (-9.56%) ⬇️
...ternal/recorder/callback/RecorderWindowCallback.kt 83.78% <50.00%> (-6.69%) ⬇️
...sessionreplay/internal/recorder/WindowInspector.kt 72.22% <71.43%> (+38.89%) ⬆️
...dog/android/sessionreplay/internal/gson/GsonExt.kt 92.59% <86.67%> (-7.41%) ⬇️
...dog/android/sessionreplay/SessionReplayRecorder.kt 94.67% <100.00%> (+0.46%) ⬆️
...oid/sessionreplay/internal/SessionReplayFeature.kt 100.00% <100.00%> (ø)
...nreplay/internal/async/RecordedDataQueueHandler.kt 97.80% <100.00%> (+9.28%) ⬆️
...lay/internal/domain/SessionReplayRequestFactory.kt 100.00% <100.00%> (ø)
...ssionreplay/internal/processor/MutationResolver.kt 89.22% <100.00%> (+0.50%) ⬆️
...replay/internal/processor/RecordedDataProcessor.kt 95.89% <100.00%> (ø)
... and 2 more

... and 17 files with indirect coverage changes

@mariusc83
mariusc83 force-pushed the mconstantin/replay-1364/use-internal-logger-in-sr-modules branch from 5a49661 to e9ee78f Compare August 11, 2023 09:04
@mariusc83
mariusc83 requested review from plousada and xgouchet August 11, 2023 09:06
jonathanmos
jonathanmos previously approved these changes Aug 13, 2023
View reviewed changes
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt Outdated
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt Outdated
@mariusc83
mariusc83 force-pushed the mconstantin/replay-1364/use-internal-logger-in-sr-modules branch 2 times, most recently from f6ddb43 to 05b69e2 Compare August 14, 2023 08:31
@mariusc83
mariusc83 force-pushed the mconstantin/replay-1364/use-internal-logger-in-sr-modules branch from 05b69e2 to a5520ed Compare August 14, 2023 09:45
@mariusc83
mariusc83 merged commit d9cc07d into develop Aug 16, 2023
@mariusc83
mariusc83 deleted the mconstantin/replay-1364/use-internal-logger-in-sr-modules branch August 16, 2023 12:04
@xgouchet xgouchet added this to the 2.1.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@plousada plousada plousada approved these changes
@jonathanmos jonathanmos jonathanmos left review comments
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mariusc83 mariusc83

Labels

None yet

Projects

None yet

Milestone

2.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@mariusc83 @codecov-commenter @xgouchet @plousada @jonathanmos