Skip to content

RUM-1820 Create full snapshot if root has changed#1567

Merged
louiszawadzki merged 1 commit into
developfrom
louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added
Jan 3, 2024
Merged

RUM-1820 Create full snapshot if root has changed#1567
louiszawadzki merged 1 commit into
developfrom
louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added

Conversation

@louiszawadzki

@louiszawadzki louiszawadzki commented Nov 23, 2023

Copy link
Copy Markdown
Contributor

What and why?

I noticed a bug in Session Replay when the root wireframe (first wireframe in the list) is added in an incremental snapshot.

This wireframe will have no previousId, and the replay will add it last in the list of wireframes.

This should not happen very often, given we create a full snapshot on every RUM view.

Possible improvements for the PR:

  • if the new root is already present in the previous wireframe, there is no need to create a new full snapshot

Also it's worth investigating why we're removing root wireframes in the first place (and thus adding them back afterwards).

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@datadog-datadog-prod-us1

Copy link
Copy Markdown

Datadog Report

Branch report: louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added
Commit report: c37b4fc

dd-sdk-ios: 1 Failed (0 Known Flaky), 0 New Flaky, 229 Passed, 0 Skipped, 3.58s Wall Time

❌ Failed Tests (1)

  • testWhenOrientationChanges_itWritesRecordsViewportResizeDataSegment - ProcessorTests - Details

    Expand for error
     Assertion Failure at ProcessorTests.swift:135: XCTAssertTrue failed
    

@maciejburda

Copy link
Copy Markdown
Member

Probably worth considering this change. WDYT @louiszawadzki @ncreated ?

@louiszawadzki
louiszawadzki force-pushed the louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added branch from c37b4fc to 8da60be Compare December 20, 2023 15:26
@louiszawadzki
louiszawadzki marked this pull request as ready for review December 20, 2023 15:26
@louiszawadzki
louiszawadzki requested review from a team as code owners December 20, 2023 15:26
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Dec 20, 2023

Copy link
Copy Markdown

Datadog Report

Branch report: louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added
Commit report: f4acf08

dd-sdk-ios: 0 Failed, 0 New Flaky, 256 Passed, 0 Skipped, 4m 53.79s Wall Time

@louiszawadzki
louiszawadzki force-pushed the louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added branch from 8da60be to f4acf08 Compare December 20, 2023 16:20
@louiszawadzki
louiszawadzki merged commit 08ddde6 into develop Jan 3, 2024
@louiszawadzki
louiszawadzki deleted the louiszawadzki/rum-1820/fix-root-wireframe-removed-and-added branch January 3, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants