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

feature/APMI-2820 iOS crash does not retain old screen name #2

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

Conversation

shattimare
Copy link

changes in crashreporting.swift

changes in crashreporting.swift
@shattimare shattimare requested a review from johnbley as a code owner March 16, 2022 12:10
@@ -84,7 +94,9 @@ func loadPendingCrashReport(_ data: Data!) throws {
exceptionType = report.exceptionInfo.exceptionName
}

let oldSessionId = String(decoding: report.customData, as: UTF8.self)
let strArr = String(decoding: report.customData, as: UTF8.self).components(separatedBy: "|")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break if the user specifiers a screen name containing a |. Please use something sensible like URL/URI encoding (which in Swift is apparently named addingPercentEncoding) and the same keys we would report with the span.

Changed logic of saving sessionid and screen name as custom data.
linting issue solved
Copy link
Collaborator

@seemk seemk left a comment

Choose a reason for hiding this comment

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

Minor comments

@@ -24,6 +24,9 @@ let CrashReportingVersionString = "0.2.0"

var TheCrashReporter: PLCrashReporter?

let keyID = "SessionID"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might rename it sessionIdKey and screenNameKey or something similar, at the moment it's a bit hard to understand at first glance what is keyID and keyName

}
}

func saveSessionIDInToCustomData() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: InTo -> Into?

minor changes
@shattimare
Copy link
Author

PR failed because of following line.
/Users/runner/work/splunk-otel-ios-crashreporting/splunk-otel-ios-crashreporting/SplunkRumCrashReporting/SplunkRumCrashReporting/CrashReporting.swift:55:15:
error:type 'SplunkRum' has no member 'addScreenNameChangeCallback'
[1762]
(https://github.com/signalfx/splunk-otel-ios-crashreporting/runs/5884155914?check_suite_focus=true#step:3:1762)
SplunkRum.addScreenNameChangeCallback { name in
[1763]
(https://github.com/signalfx/splunk-otel-ios-crashreporting/runs/5884155914?check_suite_focus=true#step:3:1763)

addScreenNameChangeCallback Is the new method which I added in SplunkRUM library as part of APMI-2820:(iOS crash does not retain old screen name ) solution, so at this time this method is not available in crash reporting library, as soon as you merge solution in Splunkrum and we will use that version in crash reporting library this error will gone.

@@ -84,7 +132,9 @@ func loadPendingCrashReport(_ data: Data!) throws {
exceptionType = report.exceptionInfo.exceptionName
}

let oldSessionId = String(decoding: report.customData, as: UTF8.self)
guard let dict = dataToDictionary(data: report.customData) else {return}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make a best effort to report data, even if we are missing some fields. Missing the old session ID and/or the old screen name should not prevent us from reporting any crash reporting span.

remove guard let , send crash span if it has not sessionid or screen name data.
@shattimare shattimare changed the title feature/APMI-2820 feature/APMI-2820 iOS crash does not retain old screen name Apr 13, 2022
shattimare and others added 2 commits April 14, 2022 11:44
test case  added to check that screen.name is there or not
-- Resolved crash occur in CrashReporting.swift.
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