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.

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

RUM-3222 Move Session Replay obj-c interfaces directly to the target #1697

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

maciejburda
Copy link
Member

What and why?

Moves obj-c interfaces of Session Replay as described in the RFC

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/

@maciejburda maciejburda marked this pull request as ready for review February 22, 2024 13:49
@maciejburda maciejburda requested review from a team as code owners February 22, 2024 13:49
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Feb 22, 2024

Datadog Report

Branch report: maciey/RUM-3222-objc-sr-wrapper
Commit report: 5047082
Test service: dd-sdk-ios

✅ 0 Failed, 2752 Passed, 0 Skipped, 6m 52.13s Wall Time
🔻 Test Sessions change in coverage: 7 decreased, 4 increased, 2 no change

🔻 Code Coverage Decreases vs Default Branch (7)

This report shows up to 5 code coverage decreases.

  • test DatadogWebViewTrackingTests iOS 18.30% (-4.52%) - Details
  • test DatadogTraceTests iOS 50.96% (-0.55%) - Details
  • test DatadogTraceTests tvOS 51.06% (-0.54%) - Details
  • test DatadogRUMTests tvOS 79.20% (-0.45%) - Details
  • test DatadogCrashReportingTests tvOS 28.43% (-0.38%) - Details

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.

💡 Moving the SR API from DatadogObjc to DatadogSessionReplay is a breaking-change, no? What is the rationale of doing it now and what is the mitigation we will offer?

@maciejburda
Copy link
Member Author

@ncreated
Truth be told, ticket originated because we missed the fact we already support obj-c in the wrapper package.

We discussed this with @maxep and it should work without a change on the customer side.

It's following the v2 idea of moving all the obj-c api directly to targets and deprecating the obj-c wrapper.

@maciejburda maciejburda force-pushed the maciey/RUM-3222-objc-sr-wrapper branch from 069b401 to e1ddbc9 Compare February 23, 2024 14:26
@maciejburda maciejburda force-pushed the maciey/RUM-3222-objc-sr-wrapper branch from e1ddbc9 to d2d153f Compare March 5, 2024 12:54
ncreated
ncreated previously approved these changes Mar 7, 2024
@maciejburda
Copy link
Member Author

Getting this in snapshot tests:

target at '~/dd-sdk-ios/DatadogSessionReplay/Tests' contains mixed language source files; feature not supported

Will try to resolve later. But sounds like it may require slightly different approach.

@maciejburda maciejburda force-pushed the maciey/RUM-3222-objc-sr-wrapper branch from 5047082 to bf02344 Compare March 22, 2024 12:43
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Mar 22, 2024

Datadog Report

Branch report: maciey/RUM-3222-objc-sr-wrapper
Commit report: 479ca23
Test service: dd-sdk-ios

✅ 0 Failed, 2897 Passed, 0 Skipped, 11m 8.86s Wall Time
🔻 Test Sessions change in coverage: 11 decreased, 3 increased

🔻 Code Coverage Decreases vs Default Branch (11)

This report shows up to 5 code coverage decreases.

  • test DatadogInternalTests iOS 79.4% (-0.36%) - Details
  • test DatadogInternalTests tvOS 79.37% (-0.34%) - Details
  • test DatadogTraceTests tvOS 49.48% (-0.26%) - Details
  • test DatadogRUMTests iOS 79.46% (-0.25%) - Details
  • test DatadogLogsTests iOS 45.7% (-0.24%) - Details

@maciejburda maciejburda force-pushed the maciey/RUM-3222-objc-sr-wrapper branch from bf02344 to 479ca23 Compare March 25, 2024 13:04
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.

🎯 nice!

@maciejburda maciejburda merged commit aa42015 into develop Mar 25, 2024
8 checks passed
@maciejburda maciejburda deleted the maciey/RUM-3222-objc-sr-wrapper branch March 25, 2024 13:53
@ncreated ncreated mentioned this pull request Apr 4, 2024
8 tasks
@maxep maxep mentioned this pull request Apr 10, 2024
8 tasks
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