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

Apple client send and receive support for crash report cohort IDs (CRCIDs) #1116

Merged
merged 32 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
88ae14a
Adopt dependency injection support for CrashCollection
jdjackson Dec 1, 2024
8583c2d
CrashCollectionTests: First test and supporting implementation nearly…
jdjackson Dec 3, 2024
24cb701
Support for CRCID send/receive
jdjackson Dec 4, 2024
418c64c
CrashCollection supports CRCID store and retrieve
jdjackson Dec 4, 2024
3b5d42a
Indentation fix
jdjackson Dec 4, 2024
1f6b78a
2 more tests implemented
jdjackson Dec 4, 2024
8440d72
Clarified remaining potential future tests
jdjackson Dec 4, 2024
4dbf3f0
Cleanup pre-code-review
jdjackson Dec 5, 2024
34ddc9c
Add support for clearing CRCID from client apps
jdjackson Dec 6, 2024
28595e2
Removing debug-only code
jdjackson Dec 6, 2024
acc46b7
Refactor CRCID storage management into new CRCIDManager class
jdjackson Dec 6, 2024
4cb1360
Linting fixes
jdjackson Dec 9, 2024
1d3a373
More linting fixes
jdjackson Dec 9, 2024
3d944d3
More linting fixes, and removing unnecessary TODOs
jdjackson Dec 9, 2024
c56f50f
More linting fixes
jdjackson Dec 9, 2024
2f921b1
Prep for pixels to detect crash reporting failures
jdjackson Dec 9, 2024
4683fb9
Getting close to a good solution for pixel tracking of failed crash s…
jdjackson Dec 12, 2024
f920d03
Removing irrelevant or infisible tests
jdjackson Dec 12, 2024
d949750
Change to submit CRCID header even when not present locally
jdjackson Dec 12, 2024
57f8280
Clean up naming of CrashReportSenderError.crcidMissing
jdjackson Dec 12, 2024
073fff9
Fixing sending of empty CRCID header when no value is present
jdjackson Dec 16, 2024
8053808
Clearing unneeded TODO
jdjackson Dec 16, 2024
e202122
Restoring proper crash.js url
jdjackson Dec 18, 2024
4fd8049
Clean up trailing whitespace for swiftlint compliance
jdjackson Dec 18, 2024
b6e731d
Code review cleanup
jdjackson Dec 19, 2024
07ca1c4
Cleaner handling of CRCID as optional
jdjackson Dec 19, 2024
15b0726
Move CRCIDManager out into its own file.
samsymons Dec 19, 2024
1c91259
Replace `print` statement with `Logger`.
samsymons Dec 19, 2024
c42eb19
Fix header comment formatting.
samsymons Dec 19, 2024
c28ed68
Use `UserDefaults.standard`.
samsymons Dec 19, 2024
88449bb
Merge branch 'main' into jjackson/crcid-send-receive-support
samsymons Dec 19, 2024
bcb85ec
Merge branch 'main' into jjackson/crcid-send-receive-support
samsymons Dec 19, 2024
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
3 changes: 2 additions & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,8 @@ let package = Package(
.testTarget(
name: "CrashesTests",
dependencies: [
"Crashes"
"Crashes",
"TestUtils"
]
),
.testTarget(
Expand Down
62 changes: 62 additions & 0 deletions Sources/Crashes/CRCIDManager.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
//
// CRCIDManager.swift
//
// Copyright © 2024 DuckDuckGo. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

import Foundation
import Persistence
import os.log

/// Cohort identifier used exclusively to distinguish systemic crashes, only after the user opts in to send them.
/// Its purpose is strictly limited to improving the reliability of crash reporting and is never used elsewhere.
public class CRCIDManager {
static let crcidKey = "CRCIDManager.crcidKey"
var store: KeyValueStoring

public init(store: KeyValueStoring = UserDefaults.standard) {
self.store = store
}

public func handleCrashSenderResult(result: Result<Data?, Error>, response: HTTPURLResponse?) {
switch result {
case .success:
Logger.general.debug("Crash Collection - Sending Crash Report: succeeded")
if let receivedCRCID = response?.allHeaderFields[CrashReportSender.httpHeaderCRCID] as? String {
if crcid != receivedCRCID {
Logger.general.debug("Crash Collection - Received new value for CRCID: \(receivedCRCID), setting local crcid value")
crcid = receivedCRCID
} else {
Logger.general.debug("Crash Collection - Received matching value for CRCID: \(receivedCRCID), no update necessary")
}
} else {
Logger.general.debug("Crash Collection - No value for CRCID header: \(CRCIDManager.crcidKey), clearing local crcid value if present")
crcid = ""
}
case .failure(let failure):
Logger.general.debug("Crash Collection - Sending Crash Report: failed (\(failure))")
}
}

public var crcid: String? {
get {
return self.store.object(forKey: CRCIDManager.crcidKey) as? String
}

set {
store.set(newValue, forKey: CRCIDManager.crcidKey)
}
}
}
37 changes: 29 additions & 8 deletions Sources/Crashes/CrashCollection.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@

import Foundation
import MetricKit
import Persistence
import os.log
import Common

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

public init(platform: CrashCollectionPlatform) {
crashHandler = CrashHandler()
crashSender = CrashReportSender(platform: platform)
public init(crashReportSender: CrashReportSending,
crashCollectionStorage: KeyValueStoring = UserDefaults.standard) {
self.crashHandler = CrashHandler()
self.crashSender = crashReportSender
self.crashCollectionStorage = crashCollectionStorage
self.crcidManager = CRCIDManager(store: crashCollectionStorage)
}

public func start(didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) {
Expand All @@ -55,7 +61,11 @@ public final class CrashCollection {
/// - didFindCrashReports: callback called after payload preprocessing is finished.
/// Provides processed JSON data to be presented to the user and Pixel parameters to fire a crash Pixel.
/// `uploadReports` callback is used when the user accepts uploading the crash report and starts crash upload to the server.
public func start(process: @escaping ([MXDiagnosticPayload]) -> [Data], didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) {
public func start(process: @escaping ([MXDiagnosticPayload]) -> [Data],
didFindCrashReports: @escaping (_ pixelParameters: [[String: String]],
_ payloads: [Data],
_ uploadReports: @escaping () -> Void) -> Void,
didFinishHandlingResponse: @escaping (() -> Void) = {}) {
let first = isFirstCrash
isFirstCrash = false

Expand All @@ -80,8 +90,13 @@ public final class CrashCollection {
didFindCrashReports(pixelParameters, processedData) {
Task {
for payload in processedData {
await self.crashSender.send(payload)
// Note: It's important that we submit crashes to our service one by one. CRCIDs are assigned (or potentially expired
// and updated with a new one) in response to a call to crash.js, and making multiple calls in parallel would mean
// the server may assign several CRCIDs to a single client in rapid succession.
let result = await self.crashSender.send(payload, crcid: self.crcidManager.crcid)
samsymons marked this conversation as resolved.
Show resolved Hide resolved
self.crcidManager.handleCrashSenderResult(result: result.result, response: result.response)
}
didFinishHandlingResponse()
}
}
}
Expand Down Expand Up @@ -171,18 +186,24 @@ public final class CrashCollection {
}, didFindCrashReports: didFindCrashReports)
}

public func clearCRCID() {
self.crcidManager.crcid = nil
}

var isFirstCrash: Bool {
get {
UserDefaults().object(forKey: Const.firstCrashKey) as? Bool ?? true
crashCollectionStorage.object(forKey: Const.firstCrashKey) as? Bool ?? true
}

set {
UserDefaults().set(newValue, forKey: Const.firstCrashKey)
crashCollectionStorage.set(newValue, forKey: Const.firstCrashKey)
}
}

let crashHandler: CrashHandler
let crashSender: CrashReportSender
let crashSender: CrashReportSending
let crashCollectionStorage: KeyValueStoring
let crcidManager: CRCIDManager

enum Const {
static let firstCrashKey = "CrashCollection.first"
Expand Down
79 changes: 71 additions & 8 deletions Sources/Crashes/CrashReportSender.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,92 @@

import Foundation
import MetricKit
import Common
import os.log

public final class CrashReportSender {
public protocol CrashReportSending {
var pixelEvents: EventMapping<CrashReportSenderError>? { get }

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)
}

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

public final class CrashReportSender: CrashReportSending {
static let reportServiceUrl = URL(string: "https://duckduckgo.com/crash.js")!

static let httpHeaderCRCID = "crcid"

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

public init(platform: CrashCollectionPlatform) {
private let session = URLSession(configuration: .ephemeral)

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

public func send(_ crashReportData: Data) async {
public func send(_ crashReportData: Data, crcid: String?, completion: @escaping (_ result: Result<Data?, Error>, _ response: HTTPURLResponse?) -> Void) {
samsymons marked this conversation as resolved.
Show resolved Hide resolved
var request = URLRequest(url: Self.reportServiceUrl)
request.setValue("text/plain", forHTTPHeaderField: "Content-Type")
request.setValue(platform.userAgent, forHTTPHeaderField: "User-Agent")

let crcidHeaderValue = crcid ?? ""
request.setValue(crcidHeaderValue, forHTTPHeaderField: CrashReportSender.httpHeaderCRCID)
Logger.general.debug("Configured crash report HTTP request with crcid: \(crcidHeaderValue)")

request.httpMethod = "POST"
request.httpBody = crashReportData

do {
_ = try await session.data(for: request)
} catch {
assertionFailure("CrashReportSender: Failed to send the crash report")
Logger.general.debug("CrashReportSender: Awaiting session data")
let task = session.dataTask(with: request) { data, response, error in
if let response = response as? HTTPURLResponse {
Logger.general.debug("CrashReportSender: Received HTTP response code: \(response.statusCode)")
if response.statusCode == 200 {
response.allHeaderFields.forEach { headerField in
Logger.general.debug("CrashReportSender: \(String(describing: headerField.key)): \(String(describing: headerField.value))")
}
let receivedCRCID = response.allHeaderFields[CrashReportSender.httpHeaderCRCID]
if receivedCRCID == nil || receivedCRCID as? String == "" {
let crashReportError = CrashReportSenderError.crcidMissing
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 {
let crashReportError = CrashReportSenderError.submissionFailed(response)
self.pixelEvents?.fire(crashReportError)
completion(.failure(crashReportError), response)
}
} else {
let crashReportError = CrashReportSenderError.submissionFailed(nil)
self.pixelEvents?.fire(crashReportError)
completion(.failure(crashReportError), nil)
}
}
task.resume()
}

private let session = URLSession(configuration: .ephemeral)
public func send(_ crashReportData: Data, crcid: String?) async -> (result: Result<Data?, Error>, response: HTTPURLResponse?) {
await withCheckedContinuation { continuation in
send(crashReportData, crcid: crcid) { result, response in
continuation.resume(returning: (result, response))
}
}
}
}
Loading
Loading