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 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Improvements

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

### Features

- Add protocol for custom screenName for UIViewControllers (#4646)
Expand Down
15 changes: 11 additions & 4 deletions Sources/Sentry/SentryClient.m
Original file line number Diff line number Diff line change
Expand Up @@ -553,10 +553,17 @@ - (void)captureReplayEvent:(SentryReplayEvent *)replayEvent
return;
}

SentryEnvelope *envelope = [[SentryEnvelope alloc]
initWithHeader:[[SentryEnvelopeHeader alloc] initWithId:replayEvent.eventId]
items:@[ videoEnvelopeItem ]];

// Hybrid SDKs may override the sdk info for a replay Event,
// the same SDK should be used for the envelope header.
SentrySdkInfo *sdkInfo = replayEvent.sdk ? [[SentrySdkInfo alloc] initWithDict:replayEvent.sdk]
: [SentrySdkInfo global];
SentryEnvelopeHeader *envelopeHeader =
[[SentryEnvelopeHeader alloc] initWithId:replayEvent.eventId
sdkInfo:sdkInfo
traceContext:nil];

SentryEnvelope *envelope = [[SentryEnvelope alloc] initWithHeader:envelopeHeader
items:@[ videoEnvelopeItem ]];
[self captureEnvelope:envelope];
}

Expand Down
1 change: 1 addition & 0 deletions Sources/Sentry/include/SentryPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#import "SentryDisplayLinkWrapper.h"
#import "SentryLevelHelper.h"
#import "SentryLogC.h"
#import "SentryMeta.h"
brustolin marked this conversation as resolved.
Show resolved Hide resolved
#import "SentryRandom.h"
#import "SentrySdkInfo.h"
#import "SentrySession.h"
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import Foundation
"errorSampleRate": options.onErrorSampleRate,
"maskAllText": options.maskAllText,
"maskAllImages": options.maskAllImages,
"quality": String(describing: options.quality)
"quality": String(describing: options.quality),
"nativeSdkName": SentryMeta.sdkName,
"nativeSdkVersion": SentryMeta.versionString
]

if !options.maskedViewClasses.isEmpty {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@_implementationOnly import _SentryPrivate
import Foundation

@objcMembers
Expand Down Expand Up @@ -146,6 +147,11 @@ public class SentryReplayOptions: NSObject, SentryRedactOptions {
*/
let maximumDuration = TimeInterval(3_600)

/**
* Used by hybrid SDKs to be able to configure SDK info for Session Replay
*/
var sdkInfo: [String: Any]?
brustolin marked this conversation as resolved.
Show resolved Hide resolved

/**
* Inittialize session replay options disabled
*/
Expand Down Expand Up @@ -183,5 +189,6 @@ public class SentryReplayOptions: NSObject, SentryRedactOptions {
if let quality = SentryReplayQuality(rawValue: dictionary["quality"] as? Int ?? -1) {
self.quality = quality
}
sdkInfo = dictionary["sdkInfo"] as? [String: Any]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

@brustolin, I think we should still align with the Java approach. Using sdkInfo as a dict is acceptable, but I think we should get rid of storing the native SDK info in the Meta.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Java is wrong in this case.
The nativeSDKName and nativeSDKVersion in the replay tags should never be overwritten.
@romtsn Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

it's up to us I guess.. most important is the version and that one will be correct (i.e. the hybrid SDKs keep the native version in there), and the name doesn't really matter, since we all know it's gonna be android or ios sdk depending on what replay you're watching

replayEvent.timestamp = video.end
replayEvent.urls = video.screens

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,25 @@ class SentrySessionReplayTests: XCTestCase {
XCTAssertFalse(fixture.displayLink.isRunning())
}

func testSdkInfoIsSet() throws {
let fixture = Fixture()
let options = SentryReplayOptions(sessionSampleRate: 1, onErrorSampleRate: 1)
options.sdkInfo = ["version": "6.0.1", "name": "sentry.test"]

let sut = fixture.getSut(options: options)
sut.start(rootView: fixture.rootView, fullSession: true)

fixture.dateProvider.advance(by: 1)
Dynamic(sut).newFrame(nil)
fixture.dateProvider.advance(by: 5)
Dynamic(sut).newFrame(nil)

let event = try XCTUnwrap(fixture.lastReplayEvent)

XCTAssertEqual(event.sdk?["version"] as? String, "6.0.1")
XCTAssertEqual(event.sdk?["name"] as? String, "sentry.test")
}

func testSaveScreenShotInBufferMode() {
let fixture = Fixture()

Expand Down Expand Up @@ -428,6 +447,8 @@ class SentrySessionReplayTests: XCTestCase {
XCTAssertNil(options["maskedViewClasses"])
XCTAssertNil(options["unmaskedViewClasses"])
XCTAssertEqual(options["quality"] as? String, "medium")
XCTAssertEqual(options["nativeSdkName"] as? String, SentryMeta.sdkName)
XCTAssertEqual(options["nativeSdkVersion"] as? String, SentryMeta.versionString)
}

func testOptionsInTheEventAllChanged() throws {
Expand Down
20 changes: 20 additions & 0 deletions Tests/SentryTests/SentryClientTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1953,6 +1953,26 @@ class SentryClientTest: XCTestCase {
XCTAssertNil(replayEvent.debugMeta)
}

func testCaptureReplayEvent_overrideEnvelopeHeaderSDKInfo() throws {
let sut = fixture.getSut()
let replayEvent = SentryReplayEvent(eventId: SentryId(), replayStartTimestamp: Date(), replayType: .session, segmentId: 2)
replayEvent.sdk = ["name": "Test SDK", "version": "1.0.0"]
let replayRecording = SentryReplayRecording(segmentId: 2, size: 200, start: Date(timeIntervalSince1970: 2), duration: 5_000, frameCount: 5, frameRate: 1, height: 930, width: 390, extraEvents: [])

//Not a video url, but its ok for test the envelope
let movieUrl = try XCTUnwrap(Bundle(for: self.classForCoder).url(forResource: "Resources/raw", withExtension: "json"))

let scope = Scope()
scope.addBreadcrumb(Breadcrumb(level: .debug, category: "Test Breadcrumb"))

sut.capture(replayEvent, replayRecording: replayRecording, video: movieUrl, with: scope)

let header = try XCTUnwrap(self.fixture.transport.sentEnvelopes.first?.header)

XCTAssertEqual(header.sdkInfo?.name, "Test SDK")
XCTAssertEqual(header.sdkInfo?.version, "1.0.0")
}

func testCaptureCrashEventSetReplayInScope() {
let sut = fixture.getSut()
let event = Event()
Expand Down
Loading