Skip to content

Commit

Permalink
Getting close to a good solution for pixel tracking of failed crash s…
Browse files Browse the repository at this point in the history 
…ubmissions
  • Loading branch information
@jdjackson
jdjackson committed Dec 12, 2024
1 parent 2f921b1 commit 4683fb9
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 23 deletions.
6 changes: 3 additions & 3 deletions Sources/Crashes/CrashCollection.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import Foundation
import MetricKit
import Persistence
import os.log
import Common

public enum CrashCollectionPlatform {
case iOS, macOS, macOSAppStore
Expand All @@ -40,7 +41,8 @@ public enum CrashCollectionPlatform {
@available(iOS 13, macOS 12, *)
public final class CrashCollection {

public init(crashReportSender: CrashReportSending, crashCollectionStorage: KeyValueStoring = UserDefaults()) {
public init(crashReportSender: CrashReportSending,
crashCollectionStorage: KeyValueStoring = UserDefaults()) {
self.crashHandler = CrashHandler()
self.crashSender = crashReportSender
self.crashCollectionStorage = crashCollectionStorage
Expand Down Expand Up @@ -232,12 +234,10 @@ public class CRCIDManager {
Logger.general.debug("Crash Collection - Received matching value for CRCID: \(receivedCRCID), no update necessary")
}
} else {
// TODO: Pixel: crashreporting_crcid-missing
Logger.general.debug("Crash Collection - No value for CRCID header: \(CRCIDManager.crcidKey), clearing local crcid value if present")
crcid = nil
}
case .failure(let failure):
// TODO: Pixel: crashreporting_submission-failed
Logger.general.debug("Crash Collection - Sending Crash Report: failed (\(failure))")
}
}
Expand Down
29 changes: 23 additions & 6 deletions Sources/Crashes/CrashReportSender.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,20 @@

import Foundation
import MetricKit
import Common

public protocol CrashReportSending {
init(platform: CrashCollectionPlatform)
var pixelEvents: EventMapping<CrashReportSenderError>? { get } // TODO: This should _not_ be optional

init(platform: CrashCollectionPlatform, pixelEvents: EventMapping<CrashReportSenderError>?)

func send(_ crashReportData: Data, crcid: String?) async -> (result: Result<Data?, Error>, response: HTTPURLResponse?)
func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ response: HTTPURLResponse?) -> Void)
}

enum CrashReportSenderError: Error {
case invalidResponse
public enum CrashReportSenderError: Error {
case noCRCID
case submissionFailed(HTTPURLResponse?)
}

// By conforming to a protocol, we can sub in mocks more easily
Expand All @@ -36,11 +41,13 @@ public final class CrashReportSender: CrashReportSending {
static let httpHeaderCRCID = "crcid"

public let platform: CrashCollectionPlatform
public var pixelEvents: EventMapping<CrashReportSenderError>?

private let session = URLSession(configuration: .ephemeral)

public init(platform: CrashCollectionPlatform) {
public init(platform: CrashCollectionPlatform, pixelEvents: EventMapping<CrashReportSenderError>?) {
self.platform = platform
self.pixelEvents = pixelEvents
}

public func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ response: HTTPURLResponse?) -> Void) {
Expand All @@ -60,19 +67,29 @@ public final class CrashReportSender: CrashReportSending {
Logger.general.debug("CrashReportSender: Received HTTP response code: \(response.statusCode)")
if response.statusCode == 200 {
response.allHeaderFields.forEach { print("\($0.key): \($0.value)") } // TODO: Why do we straight-up print these, rather than debug logging?
if response.allHeaderFields[CrashReportSender.httpHeaderCRCID] == nil {
let crashReportError = CrashReportSenderError.noCRCID
self.pixelEvents?.fire(crashReportError)
}
} else {
assertionFailure("CrashReportSender: Failed to send the crash report: \(response.statusCode)")
}

if let data {
completion(.success(data), response)
} else if let error {
let crashReportError = CrashReportSenderError.submissionFailed(response)
self.pixelEvents?.fire(crashReportError)
completion(.failure(error), response)
} else {
completion(.failure(CrashReportSenderError.invalidResponse), response)
let crashReportError = CrashReportSenderError.submissionFailed(response)
self.pixelEvents?.fire(crashReportError)
completion(.failure(crashReportError), response)
}
} else {
completion(.failure(CrashReportSenderError.invalidResponse), nil)
let crashReportError = CrashReportSenderError.submissionFailed(nil)
self.pixelEvents?.fire(crashReportError)
completion(.failure(crashReportError), nil)
}
}
task.resume()
Expand Down
89 changes: 75 additions & 14 deletions Tests/CrashesTests/CrashCollectionTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,14 @@ import MetricKit
import XCTest
import Persistence
import TestUtils
import Common

class CrashCollectionTests: XCTestCase {

func testFirstCrashFlagSent() {
let crashCollection = CrashCollection(crashReportSender: CrashReportSender(platform: .iOS), crashCollectionStorage: MockKeyValueStore())
let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: MockKeyValueStore())
// 2 pixels with first = true attached
XCTAssertTrue(crashCollection.isFirstCrash)
crashCollection.start { pixelParameters, _, _ in
Expand All @@ -42,7 +45,9 @@ class CrashCollectionTests: XCTestCase {
}

func testSubsequentPixelsDontSendFirstFlag() {
let crashCollection = CrashCollection(crashReportSender: CrashReportSender(platform: .iOS), crashCollectionStorage: MockKeyValueStore())
let crashReportSender = CrashReportSender(platform: .iOS, pixelEvents: nil)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: MockKeyValueStore())
// 2 pixels with no first parameter
crashCollection.isFirstCrash = false
crashCollection.start { pixelParameters, _, _ in
Expand All @@ -62,9 +67,10 @@ class CrashCollectionTests: XCTestCase {
let responseCRCIDValue = "CRCID Value"

let store = MockKeyValueStore()
let crashReportSender = MockCrashReportSender(platform: .iOS)
let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil)
crashReportSender.responseCRCID = responseCRCIDValue
let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: store)
let expectation = self.expectation(description: "Crash collection response")

// Set up closures on our CrashCollection object
Expand Down Expand Up @@ -93,8 +99,9 @@ class CrashCollectionTests: XCTestCase {
func testCRCIDIsClearedWhenServerReturnsSuccessWithNoCRCID()
{
let store = MockKeyValueStore()
let crashReportSender = MockCrashReportSender(platform: .iOS)
let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store)
let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: store)
let expectation = self.expectation(description: "Crash collection response")

// Set up closures on our CrashCollection object
Expand Down Expand Up @@ -123,8 +130,9 @@ class CrashCollectionTests: XCTestCase {

func testCRCIDIsRetainedWhenServerErrorIsReceived() {
let store = MockKeyValueStore()
let crashReportSender = MockCrashReportSender(platform: .iOS)
let crashCollection = CrashCollection(crashReportSender: crashReportSender, crashCollectionStorage: store)
let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: store)
let expectation = self.expectation(description: "Crash collection response")

// Set up closures on our CrashCollection object
Expand All @@ -151,7 +159,41 @@ class CrashCollectionTests: XCTestCase {

XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, crcid)
}


// TODO: This test doesn't actually test CrashCollection, it's testing the MockCrashReportSender
// Useful for debugging, but not actually a good test, and should be removed.
func testInvalidResponsePixelIsSent() {
let store = MockKeyValueStore()
let crashReportSender = MockCrashReportSender(platform: .iOS, pixelEvents: nil)
let crashCollection = CrashCollection(crashReportSender: crashReportSender,
crashCollectionStorage: store)
let expectation = self.expectation(description: "Crash collection response")

// Set up closures on our CrashCollection object
crashCollection.start(process: {_ in
return ["fake-crash-data".data(using: .utf8)!] // Not relevant to this test
}) { pixelParameters, payloads, uploadReports in
uploadReports()
} didFinishHandlingResponse: {
expectation.fulfill()
}

// Execute crash collection (which will call our mocked CrashReportSender as well)
let crcid = "Initial CRCID Value"
store.set(crcid, forKey: CRCIDManager.crcidKey)
crashReportSender.responseStatusCode = 500
crashCollection.crashHandler.didReceive([
MockPayload(mockCrashes: [
MXCrashDiagnostic(),
MXCrashDiagnostic()
])
])

self.wait(for: [expectation, crashReportSender.submissionFailedExpectation], timeout: 3)

XCTAssertEqual(store.object(forKey: CRCIDManager.crcidKey) as? String, crcid)
}

func testCRCIDIsSentToServer() {
// TODO: Requires ability to inspect outbound HTTP request
}
Expand Down Expand Up @@ -184,9 +226,27 @@ class MockCrashReportSender: CrashReportSending {
let platform: CrashCollectionPlatform
var responseCRCID: String?
var responseStatusCode = 200

required init(platform: CrashCollectionPlatform) {

// Pixel handling
var noCRCIDExpectation = XCTestExpectation(description: "No CRCID Expectation")
var submissionFailedExpectation = XCTestExpectation(description: "Submission Failed Expectation")

var pixelEvents: EventMapping<CrashReportSenderError>?

required init(platform: CrashCollectionPlatform, pixelEvents: EventMapping<CrashReportSenderError>?) {
self.platform = platform

self.pixelEvents = .init { event, _, _, _ in
switch event {
case CrashReportSenderError.noCRCID:
self.noCRCIDExpectation.fulfill()
return

case CrashReportSenderError.submissionFailed(_):
self.submissionFailedExpectation.fulfill()
return
}
}
}

func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ response: HTTPURLResponse?) -> Void) {
Expand All @@ -204,11 +264,12 @@ class MockCrashReportSender: CrashReportSending {
}

if responseStatusCode == 200 {
completion(.success(nil), response)
completion(.success(nil), response) // Success with nil data
} else {
completion(.failure(CrashReportSenderError.invalidResponse), response)
let crashReportError = CrashReportSenderError.submissionFailed(response)
self.pixelEvents?.fire(crashReportError)
completion(.failure(CrashReportSenderError.submissionFailed(response)), response)
}

}

func send(_ crashReportData: Data, crcid: String?) async -> (result: Result<Data?, Error>, response: HTTPURLResponse?) {
Expand Down

0 comments on commit 4683fb9

Please sign in to comment.