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

Feat: Add native SDK information in the replay option event #4663

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Dec 23, 2024

📜 Description

Added cocoa name and version in the session replay options event for easy investigation.

image

💚 How did you test it?

Unit test

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

github-actions bot commented Dec 23, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 4a4126a

CHANGELOG.md Outdated Show resolved Hide resolved
@brustolin brustolin mentioned this pull request Dec 23, 2024
7 tasks
Copy link

github-actions bot commented Dec 23, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.84 ms 1256.41 ms 19.56 ms
Size 22.31 KiB 761.40 KiB 739.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dd4145f 1233.48 ms 1254.35 ms 20.88 ms
6604dbb 1248.35 ms 1256.14 ms 7.79 ms
b4f8dba 1343.92 ms 1362.96 ms 19.04 ms
a0cc9d6 1232.37 ms 1249.55 ms 17.18 ms
3cba0e8 1250.86 ms 1258.39 ms 7.53 ms
fa38a2b 1217.92 ms 1235.52 ms 17.60 ms
728804f 1341.16 ms 1371.27 ms 30.10 ms
bef2003 1248.18 ms 1258.86 ms 10.68 ms
7f14650 1236.00 ms 1255.66 ms 19.66 ms
b9b0f0a 1251.45 ms 1257.86 ms 6.41 ms

App size

Revision Plain With Sentry Diff
dd4145f 21.58 KiB 540.09 KiB 518.51 KiB
6604dbb 22.84 KiB 402.56 KiB 379.72 KiB
b4f8dba 21.58 KiB 614.87 KiB 593.29 KiB
a0cc9d6 21.58 KiB 706.46 KiB 684.88 KiB
3cba0e8 22.84 KiB 403.19 KiB 380.34 KiB
fa38a2b 21.58 KiB 418.70 KiB 397.12 KiB
728804f 22.85 KiB 411.75 KiB 388.91 KiB
bef2003 22.85 KiB 407.73 KiB 384.88 KiB
7f14650 22.84 KiB 402.63 KiB 379.78 KiB
b9b0f0a 20.76 KiB 434.94 KiB 414.18 KiB

Previous results on branch: feat/sdk-info-sr-tags

Startup times

Revision Plain With Sentry Diff
b7ac501 1230.59 ms 1253.66 ms 23.07 ms
37adb91 1240.06 ms 1261.49 ms 21.43 ms
7cdca55 1240.24 ms 1256.24 ms 16.00 ms
32a98c1 1227.61 ms 1252.87 ms 25.26 ms

App size

Revision Plain With Sentry Diff
b7ac501 22.32 KiB 761.95 KiB 739.63 KiB
37adb91 22.31 KiB 761.39 KiB 739.08 KiB
7cdca55 22.32 KiB 761.54 KiB 739.22 KiB
32a98c1 22.32 KiB 762.11 KiB 739.79 KiB

@brustolin brustolin marked this pull request as draft December 23, 2024 16:23
@brustolin brustolin marked this pull request as ready for review December 24, 2024 08:44
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.670%. Comparing base (c5fef16) to head (4a4126a).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4663       +/-   ##
=============================================
+ Coverage   90.631%   90.670%   +0.038%     
=============================================
  Files          621       621               
  Lines        71153     71191       +38     
  Branches     25310     25936      +626     
=============================================
+ Hits         64487     64549       +62     
+ Misses        6574      6545       -29     
- Partials        92        97        +5     
Files with missing lines Coverage Δ
Sources/Sentry/SentryMeta.m 72.727% <100.000%> (+22.727%) ⬆️
.../SessionReplay/RRWeb/SentryRRWebOptionsEvent.swift 100.000% <100.000%> (ø)
...tegrations/SessionReplay/SentryReplayOptions.swift 95.000% <100.000%> (+0.084%) ⬆️
...tegrations/SessionReplay/SentrySessionReplay.swift 97.619% <100.000%> (+0.009%) ⬆️
...tions/SessionReplay/SentrySessionReplayTests.swift 99.111% <100.000%> (+0.041%) ⬆️

... and 19 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5fef16...4a4126a. Read the comment docs.

Copy link
Contributor

@philprime philprime left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -248,6 +248,7 @@ class SentrySessionReplay: NSObject {
private func captureSegment(segment: Int, video: SentryVideoInfo, replayId: SentryId, replayType: SentryReplayType) {
let replayEvent = SentryReplayEvent(eventId: replayId, replayStartTimestamp: video.start, replayType: replayType, segmentId: segment)

replayEvent.sdk = self.replayOptions.sdkInfo
Copy link
Member

Choose a reason for hiding this comment

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

we should also set the same sdkInfo to the envelope header, which this replay event is a part of. And from what I see we don't set this sdkInfo to the default global one when initialized, which means we'll nullify the sdk context if we're not running on a hybrid SDK, right? @philprime @philipphofmann could you guys do that or should I take care while Dhiogo is out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I know how to do that yet, would be great if you or @philipphofmann can do it, with me reviewing the changes so I know afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this PR; see #4663 (review).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This is PR highly confusing to me. The SentryMeta sdkName and SDKVersion can be changed, and only SR events would use the nativeVersionString and nativeSDKName. All other Sentry data uses the SentryMeta sdkName and sdkVersion. @romtsn, do you use a similar approach on Android? If yes, what are the reasons for that?

Comment on lines +16 to +17
versionString = sentryVersionString.copy;
sdkName = sentrySdkName.copy;
Copy link
Member

Choose a reason for hiding this comment

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

m: It's a good practice to do this check to avoid getting called multiple times in case we ever subclass this, see https://developer.apple.com/documentation/objectivec/nsobject/1418639-initialize.

if (self == [SentryMeta class]) {

}

/**
* Used by hybrid SDKs to be able to configure SDK info for Session Replay
*/
var sdkInfo: [String: Any]?
Copy link
Member

Choose a reason for hiding this comment

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

m: We should set the intial value to the values of the SentryMeta.

Copy link
Member

Choose a reason for hiding this comment

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

m: We should use SentrySDKInfo here, convert it to Swift, and make it public. That's also how Android does it. Unfortunately, that's not trivial.

@@ -15,6 +15,7 @@

### Improvements

- Add native SDK information in the replay option event (#4663)
Copy link

Choose a reason for hiding this comment

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

  • 🚫 The changelog entry seems to be part of an already released section ## 8.43.0.
    Consider moving the entry to the ## Unreleased section, please.

@kahest kahest mentioned this pull request Jan 8, 2025
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.

5 participants