Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RUM-7316 Session Replay SwiftUI Feature Flag #2124

Merged
merged 3 commits into from
Nov 28, 2024
Merged

RUM-7316 Session Replay SwiftUI Feature Flag #2124

merged 3 commits into from
Nov 28, 2024

Conversation

maxep
Copy link
Member

@maxep maxep commented Nov 28, 2024

What and why?

Enable SwiftUI recording using a feature-flag

How?

Defines SessionReplay.Configuration.FeatureFlags with .swiftui disabled by default.

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
  • Add Objective-C interface for public APIs (see our guidelines [internal]) and run make api-surface)

@maxep maxep requested review from a team as code owners November 28, 2024 12:15
@mobile-app-ci mobile-app-ci force-pushed the maxep/RUM-7316/swiftui-ff branch from 040a14d to 9faa288 Compare November 28, 2024 12:24
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Nov 28, 2024 •
edited
Loading

Datadog Report

Branch report: maxep/RUM-7316/swiftui-ff
Commit report: c78b236
Test service: dd-sdk-ios

✅ 0 Failed, 3561 Passed, 0 Skipped, 2m 21.83s Total Time
🔻 Test Sessions change in coverage: 2 decreased, 4 increased, 8 no change

🔻 Code Coverage Decreases vs Default Branch (2)

  • test DatadogTraceTests iOS 54.19% (-0.03%) - Details
  • test DatadogInternalTests tvOS 80.39% (-0.02%) - Details

mariedm
mariedm reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@mariedm mariedm left a comment

Choose a reason for hiding this comment

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

Nice work! 🙌

DatadogSessionReplay/Sources/SessionReplayConfiguration.swift
@@ -81,20 +84,23 @@ extension SessionReplay {
/// - touchPrivacyLevel: The way user touches (e.g. tap) should be masked. Default: `.hide`.
/// - startRecordingImmediately: If the recording should start automatically. When `true`, the recording starts automatically; when `false` it doesn't, and the recording will need to be started manually. Default: `true`.
/// - customEndpoint: Custom server url for sending replay data. Default: `nil`.
/// - featureFlags: Experimental feature flags.
Copy link
Member

Choose a reason for hiding this comment

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

Do we intentionally not add this param to the deprecated API? I think it makes sense, but double-checking just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it intentionally, yes. I think it makes sense to not add features to deprecated api 👍

...lay/Sources/Recorder/ViewTreeSnapshotProducer/ViewTreeSnapshot/ViewTreeSnapshotBuilder.swift Outdated Show resolved Hide resolved
...corder/ViewTreeSnapshotProducer/ViewTreeSnapshot/NodeRecorders/UnsupportedViewRecorder.swift Outdated
]
// swiftlint:enable opening_brace
// disable swiftui based on ff
if !featureFlags[.swiftui, default: false] {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could make it a bit easier to read with a computer property

extension SessionReplay.Configuration.FeatureFlags {
    var isSwiftUIDisabled: Bool {
        return !self[.swiftui, default: false]
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I have something more generic:

public subscript(flag: Key) -> Bool {
    self[flag, default: false]
}

@maxep maxep changed the base branch from maxep/sr/swift-ui to develop November 28, 2024 14:47
@mobile-app-ci mobile-app-ci force-pushed the maxep/RUM-7316/swiftui-ff branch from 9faa288 to a1aaee1 Compare November 28, 2024 14:48
@maxep maxep requested a review from mariedm November 28, 2024 14:53
@maxep
Copy link
Member Author

maxep commented Nov 28, 2024

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 28, 2024 •
edited
Loading

Devflow running: /merge

View all feedbacks in Devflow UI.

2024-11-28 16:12:00 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-11-28 16:16:16 UTC ℹ️ MergeQueue: merge request added to the queue

The median merge time in develop is 28m.

@dd-mergequeue dd-mergequeue bot merged commit b90f78c into develop Nov 28, 2024
16 checks passed
@mariedm mariedm deleted the maxep/RUM-7316/swiftui-ff branch December 2, 2024 12:38
@mariedm mariedm mentioned this pull request Dec 10, 2024
Merged
4 tasks
This was referenced Dec 11, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariedm mariedm mariedm approved these changes

Assignees
No one assigned
Labels
mergequeue-status: done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxep @mariedm