Skip to content

feat: Add iOS overhead for SessionReplayFeature #749

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.

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

fuzzybinary
Copy link
Member

What and why?

This adds a Feature that will pass through Flutter generated Session Replay Records into SRSegments. The iOS Feature and RequestBuilder take care of serializing data to disk, assembling and compressing the SRSegments to send to intake.

The feature also takes care of broadcasting RUM context changes back to Flutter.

refs: RUM-9551

Review checklist

  • This pull request has appropriate unit and / or integration tests
  • This pull request references a Github or JIRA issue

@fuzzybinary fuzzybinary force-pushed the jward/RUM-9551-sr-ios-overhead branch from 6e26be7 to 5d3f972 Compare April 30, 2025 13:36
This adds a Feature that will pass through Flutter generated Session Replay Records into SRSegments. The iOS Feature and RequestBuilder take care of serializing data to disk, assembling and compressing the SRSegments to send to intake.

The feature also takes care of broadcasting RUM context changes back to Flutter.

refs: RUM-9551
@fuzzybinary fuzzybinary force-pushed the jward/RUM-9551-sr-ios-overhead branch from 5d3f972 to 622893b Compare April 30, 2025 13:43
@fuzzybinary fuzzybinary marked this pull request as ready for review April 30, 2025 13:43
@fuzzybinary fuzzybinary requested a review from a team as a code owner April 30, 2025 13:43
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks good 👍. Please coordinate with @maxep to ensure there are no conflicts with the concurrent changes to DatadogInternal in dd-sdk-ios.

The code imported from dd-sdk-ios is appropriate for now 👍. However, for long-term maintainability, it might be worth exploring whether some of this logic can be reused via a separate library or through SessionReplay._internal.* extension helpers in native SR. Alternatively, we could use DatadogInternal, but it will lead to unnecessary coupling by introducing session replay-specific internals there.

feature?.setHasReplay(hasReplay)

result(nil)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +19 to +20
/// The RUM context received from `DatadogCore`.
internal struct RUMContext: Codable, Equatable {
Copy link
Member

Choose a reason for hiding this comment

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

💡 There's parallel work being done by @maxep around type-sharing through DatadogInternal. It covers RUM types (DataDog/dd-sdk-ios#2276). You both may want to sync on how to land both changes seamlessly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the featureBaggages in the core context are not yet touched, but they will be replaced by strongly typed definitions very soon. It will be a breaking change for this integration but an easy one. @fuzzybinary, I will ping you once done 👍

let requestBuilder: DatadogInternal.FeatureRequestBuilder
let messageReceiver: DatadogInternal.FeatureMessageReceiver

private weak var core: DatadogCoreProtocol?
Copy link
Member

Choose a reason for hiding this comment

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

convention/ It is discouraged to retain reference to core even if weak. It is best to retain FeatureScope:

let scope = core.scope(for: FlutterSessionReplayFeature.self)

The FeatureScope is already thread safe, non-optional and will be loaded lazily if it's requested before the feature is actually registered.

Comment on lines +37 to +40
} catch let error {
print("Error \(error)")
throw error
}
Copy link
Member

Choose a reason for hiding this comment

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

nit/ Is this print() intentional? Otherwise this block doesn't seem to do much.

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.

3 participants