Improved bug report error handling (#1018)

* Improve rageshake error handling.

* Add a max file size and exclude older files one the limit is hit.

* Zip log files as smaller chunks.
This commit is contained in:
Doug 2023-06-06 09:24:01 +01:00 committed by GitHub
parent 4e926c3334
commit ea4aa943e0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 279 additions and 100 deletions

View File

@ -125,6 +125,8 @@ final class AppSettings {
// Use the name allocated by the bug report server
let bugReportApplicationId = "element-x-ios"
let bugReportUISIId = "element-auto-uisi"
/// The maximum size of the upload request. Default value is just below CloudFlare's max request size.
let bugReportMaxUploadSize = 50 * 1024 * 1024
// MARK: - Analytics

View File

@ -172,25 +172,21 @@ class BugReportServiceMock: BugReportServiceProtocol {
}
//MARK: - submitBugReport
var submitBugReportProgressListenerThrowableError: Error?
var submitBugReportProgressListenerCallsCount = 0
var submitBugReportProgressListenerCalled: Bool {
return submitBugReportProgressListenerCallsCount > 0
}
var submitBugReportProgressListenerReceivedArguments: (bugReport: BugReport, progressListener: ProgressListener?)?
var submitBugReportProgressListenerReceivedInvocations: [(bugReport: BugReport, progressListener: ProgressListener?)] = []
var submitBugReportProgressListenerReturnValue: SubmitBugReportResponse!
var submitBugReportProgressListenerClosure: ((BugReport, ProgressListener?) async throws -> SubmitBugReportResponse)?
var submitBugReportProgressListenerReturnValue: Result<SubmitBugReportResponse, BugReportServiceError>!
var submitBugReportProgressListenerClosure: ((BugReport, ProgressListener?) async -> Result<SubmitBugReportResponse, BugReportServiceError>)?
func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async throws -> SubmitBugReportResponse {
if let error = submitBugReportProgressListenerThrowableError {
throw error
}
func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async -> Result<SubmitBugReportResponse, BugReportServiceError> {
submitBugReportProgressListenerCallsCount += 1
submitBugReportProgressListenerReceivedArguments = (bugReport: bugReport, progressListener: progressListener)
submitBugReportProgressListenerReceivedInvocations.append((bugReport: bugReport, progressListener: progressListener))
if let submitBugReportProgressListenerClosure = submitBugReportProgressListenerClosure {
return try await submitBugReportProgressListenerClosure(bugReport, progressListener)
return await submitBugReportProgressListenerClosure(bugReport, progressListener)
} else {
return submitBugReportProgressListenerReturnValue
}

View File

@ -126,23 +126,29 @@ class MXLogger {
}
}
/// The list of all log file URLs.
/// The list of all log file URLs, sorted chronologically.
static var logFiles: [URL] {
var logFiles = [URL]()
var logFiles = [(url: URL, modificationDate: Date)]()
let fileManager = FileManager.default
let enumerator = fileManager.enumerator(at: logsFolderURL, includingPropertiesForKeys: nil)
let enumerator = fileManager.enumerator(at: logsFolderURL, includingPropertiesForKeys: [.contentModificationDateKey])
// Find all *.log files
// Find all *.log files and their modification dates.
while let logURL = enumerator?.nextObject() as? URL {
if logURL.lastPathComponent.hasPrefix("console") {
logFiles.append(logURL)
guard let resourceValues = try? logURL.resourceValues(forKeys: [.contentModificationDateKey]),
let modificationDate = resourceValues.contentModificationDate
else { continue }
if logURL.pathExtension == "log" {
logFiles.append((logURL, modificationDate))
}
}
MXLog.info("logFiles: \(logFiles)")
let sortedFiles = logFiles.sorted { $0.modificationDate > $1.modificationDate }.map(\.url)
return logFiles
MXLog.info("logFiles: \(sortedFiles.map(\.lastPathComponent))")
return sortedFiles
}
// MARK: - Exceptions and crashes

View File

@ -100,6 +100,6 @@ final class BugReportScreenCoordinator: CoordinatorProtocol {
}
private func showError(label: String) {
parameters.userIndicatorController?.submitIndicator(UserIndicator(title: label))
parameters.userIndicatorController?.submitIndicator(UserIndicator(title: label, iconName: "xmark"))
}
}

View File

@ -64,27 +64,35 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie
private func submitBugReport() async {
let progressTracker = ProgressTracker()
actionsSubject.send(.submitStarted(progressTracker: progressTracker))
do {
var files: [URL] = []
if let screenshot = context.viewState.screenshot {
let imageURL = URL.temporaryDirectory.appendingPathComponent("Screenshot.png")
let pngData = screenshot.pngData()
try pngData?.write(to: imageURL)
var files: [URL] = []
if let screenshot = context.viewState.screenshot,
let pngData = screenshot.pngData() {
let imageURL = URL.temporaryDirectory.appendingPathComponent("Screenshot.png")
do {
try pngData.write(to: imageURL)
files.append(imageURL)
} catch {
MXLog.error("Failed writing screenshot to disk")
// Continue anyway without the screenshot.
}
let bugReport = BugReport(userID: userID,
deviceID: deviceID,
text: context.reportText,
includeLogs: context.sendingLogsEnabled,
includeCrashLog: true,
githubLabels: [],
files: files)
let result = try await bugReportService.submitBugReport(bugReport,
progressListener: progressTracker)
MXLog.info("SubmitBugReport succeeded, result: \(result.reportUrl)")
}
let bugReport = BugReport(userID: userID,
deviceID: deviceID,
text: context.reportText,
includeLogs: context.sendingLogsEnabled,
includeCrashLog: true,
githubLabels: [],
files: files)
switch await bugReportService.submitBugReport(bugReport,
progressListener: progressTracker) {
case .success(let response):
MXLog.info("Submission uploaded to: \(response.reportUrl)")
actionsSubject.send(.submitFinished)
} catch {
MXLog.error("SubmitBugReport failed: \(error)")
case .failure(let error):
MXLog.error("Submission failed: \(error)")
actionsSubject.send(.submitFailed(error: error))
}
}

View File

@ -24,6 +24,7 @@ class BugReportService: NSObject, BugReportServiceProtocol {
private let baseURL: URL
private let sentryURL: URL
private let applicationId: String
private let maxUploadSize: Int
private let session: URLSession
private var lastCrashEventId: String?
private let progressSubject = PassthroughSubject<Double, Never>()
@ -32,10 +33,12 @@ class BugReportService: NSObject, BugReportServiceProtocol {
init(withBaseURL baseURL: URL,
sentryURL: URL,
applicationId: String = ServiceLocator.shared.settings.bugReportApplicationId,
maxUploadSize: Int = ServiceLocator.shared.settings.bugReportMaxUploadSize,
session: URLSession = .shared) {
self.baseURL = baseURL
self.sentryURL = sentryURL
self.applicationId = applicationId
self.maxUploadSize = maxUploadSize
self.session = session
super.init()
@ -95,8 +98,9 @@ class BugReportService: NSObject, BugReportServiceProtocol {
SentrySDK.crash()
}
// swiftlint:disable:next function_body_length
func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async throws -> SubmitBugReportResponse {
// swiftlint:disable:next function_body_length cyclomatic_complexity
func submitBugReport(_ bugReport: BugReport,
progressListener: ProgressListener?) async -> Result<SubmitBugReportResponse, BugReportServiceError> {
var params = [
MultipartFormData(key: "user_id", type: .text(value: bugReport.userID)),
MultipartFormData(key: "text", type: .text(value: bugReport.text))
@ -111,13 +115,11 @@ class BugReportService: NSObject, BugReportServiceProtocol {
for label in bugReport.githubLabels {
params.append(MultipartFormData(key: "label", type: .text(value: label)))
}
let zippedFiles = try await zipFiles(includeLogs: bugReport.includeLogs,
includeCrashLog: bugReport.includeCrashLog)
// log or compressed-log
if !zippedFiles.isEmpty {
for url in zippedFiles {
params.append(MultipartFormData(key: "compressed-log", type: .file(url: url)))
}
let logAttachments = await zipFiles(includeLogs: bugReport.includeLogs,
includeCrashLog: bugReport.includeCrashLog)
for url in logAttachments.files {
params.append(MultipartFormData(key: "compressed-log", type: .file(url: url)))
}
if let crashEventId = lastCrashEventId {
@ -131,7 +133,12 @@ class BugReportService: NSObject, BugReportServiceProtocol {
let boundary = "Boundary-\(UUID().uuidString)"
var body = Data()
for param in params {
try body.appendParam(param, boundary: boundary)
do {
try body.appendParam(param, boundary: boundary)
} catch {
MXLog.error("Failed to attach parameter at \(param.key)")
// Continue to the next parameter and try to submit something.
}
}
body.appendString(string: "--\(boundary)--\r\n")
@ -141,30 +148,48 @@ class BugReportService: NSObject, BugReportServiceProtocol {
request.httpMethod = "POST"
request.httpBody = body as Data
let data: Data
var delegate: URLSessionTaskDelegate?
if let progressListener {
progressSubject
.receive(on: DispatchQueue.main)
.weakAssign(to: \.value, on: progressListener.progressSubject)
.store(in: &cancellables)
(data, _) = try await session.data(for: request, delegate: self)
} else {
(data, _) = try await session.data(for: request)
}
// Parse the JSON data
let decoder = JSONDecoder()
decoder.keyDecodingStrategy = .convertFromSnakeCase
let result = try decoder.decode(SubmitBugReportResponse.self, from: data)
if !result.reportUrl.isEmpty {
MXLogger.deleteCrashLog()
lastCrashEventId = nil
delegate = self
}
MXLog.info("Feedback submitted.")
return result
do {
let (data, response) = try await session.dataWithRetry(for: request, delegate: delegate)
guard let httpResponse = response as? HTTPURLResponse else {
let errorDescription = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "Unknown"
MXLog.error("Failed to submit bug report: \(errorDescription)")
MXLog.error("Response: \(response)")
return .failure(.serverError(response, errorDescription))
}
guard httpResponse.statusCode == 200 else {
let errorDescription = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "Unknown"
MXLog.error("Failed to submit bug report: \(errorDescription) (\(httpResponse.statusCode))")
MXLog.error("Response: \(httpResponse)")
return .failure(.httpError(httpResponse, errorDescription))
}
// Parse the JSON data
let decoder = JSONDecoder()
decoder.keyDecodingStrategy = .convertFromSnakeCase
let uploadResponse = try decoder.decode(SubmitBugReportResponse.self, from: data)
if !uploadResponse.reportUrl.isEmpty {
MXLogger.deleteCrashLog()
lastCrashEventId = nil
}
MXLog.info("Feedback submitted.")
return .success(uploadResponse)
} catch {
return .failure(.uploadFailure(error))
}
}
// MARK: - Private
@ -200,7 +225,7 @@ class BugReportService: NSObject, BugReportServiceProtocol {
}
private func zipFiles(includeLogs: Bool,
includeCrashLog: Bool) async throws -> [URL] {
includeCrashLog: Bool) async -> Logs {
MXLog.info("zipFiles: includeLogs: \(includeLogs), includeCrashLog: \(includeCrashLog)")
var filesToCompress: [URL] = []
@ -210,37 +235,69 @@ class BugReportService: NSObject, BugReportServiceProtocol {
if includeCrashLog, let crashLogFile = MXLogger.crashLog {
filesToCompress.append(crashLogFile)
}
var totalSize = 0
var totalZippedSize = 0
var zippedFiles: [URL] = []
var compressedLogs = Logs(maxFileSize: maxUploadSize)
for url in filesToCompress {
let zippedFileURL = URL.temporaryDirectory
.appendingPathComponent(url.lastPathComponent)
// remove old zipped file if exists
try? FileManager.default.removeItem(at: zippedFileURL)
let rawData = try Data(contentsOf: url)
if rawData.isEmpty {
continue
do {
try attachFile(at: url, to: &compressedLogs)
} catch {
MXLog.error("Failed to compress log at \(url)")
// Continue so that other logs can still be sent.
}
guard let zippedData = (rawData as NSData).gzipped() else {
continue
}
totalSize += rawData.count
totalZippedSize += zippedData.count
try zippedData.write(to: zippedFileURL)
zippedFiles.append(zippedFileURL)
}
MXLog.info("zipFiles: originalSize: \(compressedLogs.originalSize), zippedSize: \(compressedLogs.zippedSize)")
MXLog.info("zipFiles: totalSize: \(totalSize), totalZippedSize: \(totalZippedSize)")
return zippedFiles
return compressedLogs
}
/// Zips a file creating chunks based on 10MB inputs.
private func attachFile(at url: URL, to zippedFiles: inout Logs) throws {
let fileHandle = try FileHandle(forReadingFrom: url)
var chunkIndex = -1
while let data = try fileHandle.read(upToCount: 10 * 1024 * 1024) {
do {
chunkIndex += 1
if let zippedData = (data as NSData).gzipped() {
let zippedFilename = url.deletingPathExtension().lastPathComponent + "_\(chunkIndex).log"
let chunkURL = URL.temporaryDirectory.appending(path: zippedFilename)
// Remove old zipped file if exists
try? FileManager.default.removeItem(at: chunkURL)
try zippedData.write(to: chunkURL)
zippedFiles.appendFile(at: chunkURL, zippedSize: zippedData.count, originalSize: data.count)
}
} catch {
MXLog.error("Failed attaching log chunk \(chunkIndex) from (\(url.lastPathComponent)")
continue
}
}
}
/// A collection of logs to be uploaded to the bug report service.
struct Logs {
/// The maximum total size of all the files.
let maxFileSize: Int
/// The files included.
private(set) var files: [URL] = []
/// The total size of the files after compression.
private(set) var zippedSize = 0
/// The original size of the files.
private(set) var originalSize = 0
mutating func appendFile(at url: URL, zippedSize: Int, originalSize: Int) {
guard self.zippedSize + zippedSize < maxFileSize else {
MXLog.error("Logs too large, skipping attachment: \(url.lastPathComponent)")
return
}
files.append(url)
self.originalSize += originalSize
self.zippedSize += zippedSize
}
}
}
@ -285,3 +342,14 @@ extension BugReportService: URLSessionTaskDelegate {
.store(in: &cancellables)
}
}
private extension URLSession {
/// The same as `data(for:delegate:)` but with an additional immediate retry if the first attempt fails.
func dataWithRetry(for request: URLRequest, delegate: URLSessionTaskDelegate? = nil) async throws -> (Data, URLResponse) {
if let firstTryResult = try? await data(for: request, delegate: delegate) {
return firstTryResult
}
return try await data(for: request, delegate: delegate)
}
}

View File

@ -31,6 +31,23 @@ struct SubmitBugReportResponse: Decodable {
var reportUrl: String
}
enum BugReportServiceError: LocalizedError {
case uploadFailure(Error)
case serverError(URLResponse, String)
case httpError(HTTPURLResponse, String)
var errorDescription: String? {
switch self {
case .uploadFailure(let error):
return error.localizedDescription
case .serverError(_, let errorDescription):
return errorDescription
case .httpError(_, let errorDescription):
return errorDescription
}
}
}
// sourcery: AutoMockable
protocol BugReportServiceProtocol {
var isRunning: Bool { get }
@ -46,5 +63,5 @@ protocol BugReportServiceProtocol {
func crash()
func submitBugReport(_ bugReport: BugReport,
progressListener: ProgressListener?) async throws -> SubmitBugReportResponse
progressListener: ProgressListener?) async -> Result<SubmitBugReportResponse, BugReportServiceError>
}

View File

@ -24,7 +24,7 @@ class BugReportServiceTests: XCTestCase {
override func setUpWithError() throws {
bugReportService = BugReportServiceMock()
bugReportService.underlyingCrashedLastRun = false
bugReportService.submitBugReportProgressListenerReturnValue = SubmitBugReportResponse(reportUrl: "https://www.example.com/123")
bugReportService.submitBugReportProgressListenerReturnValue = .success(SubmitBugReportResponse(reportUrl: "https://www.example.com/123"))
}
func testInitialStateWithMockService() {
@ -39,8 +39,8 @@ class BugReportServiceTests: XCTestCase {
includeCrashLog: true,
githubLabels: [],
files: [])
let result = try await bugReportService.submitBugReport(bugReport, progressListener: nil)
XCTAssertFalse(result.reportUrl.isEmpty)
let response = try await bugReportService.submitBugReport(bugReport, progressListener: nil).get()
XCTAssertFalse(response.reportUrl.isEmpty)
}
func testInitialStateWithRealService() throws {
@ -64,17 +64,43 @@ class BugReportServiceTests: XCTestCase {
includeCrashLog: true,
githubLabels: [],
files: [])
let result = try await service.submitBugReport(bugReport, progressListener: nil)
let response = try await service.submitBugReport(bugReport, progressListener: nil).get()
XCTAssertEqual(result.reportUrl, "https://example.com/123")
XCTAssertEqual(response.reportUrl, "https://example.com/123")
}
func testLogsMaxSize() {
// Given a new set of logs
var logs = BugReportService.Logs(maxFileSize: 1000)
XCTAssertEqual(logs.zippedSize, 0)
XCTAssertEqual(logs.originalSize, 0)
XCTAssertTrue(logs.files.isEmpty)
// When adding new files within the size limit
logs.appendFile(at: .homeDirectory, zippedSize: 250, originalSize: 1000)
logs.appendFile(at: .picturesDirectory, zippedSize: 500, originalSize: 2000)
// Then the logs should be included
XCTAssertEqual(logs.zippedSize, 750)
XCTAssertEqual(logs.originalSize, 3000)
XCTAssertEqual(logs.files, [.homeDirectory, .picturesDirectory])
// When adding a new file larger that will exceed the size limit
logs.appendFile(at: .homeDirectory, zippedSize: 500, originalSize: 2000)
// Then the files shouldn't be included.
XCTAssertEqual(logs.zippedSize, 750)
XCTAssertEqual(logs.originalSize, 3000)
XCTAssertEqual(logs.files, [.homeDirectory, .picturesDirectory])
}
}
private class MockURLProtocol: URLProtocol {
override func startLoading() {
let response = "{\"report_url\":\"https://example.com/123\"}"
if let data = response.data(using: .utf8) {
let urlResponse = URLResponse()
if let data = response.data(using: .utf8),
let url = request.url,
let urlResponse = HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil) {
client?.urlProtocol(self, didReceive: urlResponse, cacheStoragePolicy: .allowedInMemoryOnly)
client?.urlProtocol(self, didLoad: data)
client?.urlProtocolDidFinishLoading(self)

View File

@ -64,7 +64,7 @@ class BugReportViewModelTests: XCTestCase {
let mockService = BugReportServiceMock()
mockService.submitBugReportProgressListenerClosure = { _, _ in
await Task.yield()
return SubmitBugReportResponse(reportUrl: "https://test.test")
return .success(SubmitBugReportResponse(reportUrl: "https://test.test"))
}
let viewModel = BugReportScreenViewModel(bugReportService: mockService,
userID: "@mock.client.com",
@ -90,7 +90,7 @@ class BugReportViewModelTests: XCTestCase {
func testSendReportWithError() async throws {
let mockService = BugReportServiceMock()
mockService.submitBugReportProgressListenerClosure = { _, _ in
throw TestError.testError
.failure(.uploadFailure(TestError.testError))
}
let viewModel = BugReportScreenViewModel(bugReportService: mockService,
userID: "@mock.client.com",

View File

@ -161,6 +161,61 @@ class LoggingTests: XCTestCase {
XCTAssertTrue(logFile.lastPathComponent.contains(target))
}
func testLogFileSorting() async throws {
// Given a collection of log files.
XCTAssertTrue(MXLogger.logFiles.isEmpty)
// When creating new logs.
let logsFileDirectory = URL.appGroupContainerDirectory
for i in 1...5 {
let filename = "console.\(i).log"
try "console".write(to: logsFileDirectory.appending(path: filename), atomically: true, encoding: .utf8)
}
for i in 1...5 {
let nseFilename = "console-nse.\(i).log"
try "nse".write(to: logsFileDirectory.appending(path: nseFilename), atomically: true, encoding: .utf8)
}
// Then the logs should be sorted chronologically (newest first) and not alphabetically.
XCTAssertEqual(MXLogger.logFiles.map(\.lastPathComponent),
["console-nse.5.log",
"console-nse.4.log",
"console-nse.3.log",
"console-nse.2.log",
"console-nse.1.log",
"console.5.log",
"console.4.log",
"console.3.log",
"console.2.log",
"console.1.log"])
// When updating the oldest log file.
let currentLogFile = logsFileDirectory.appending(path: "console.1.log")
let fileHandle = try FileHandle(forWritingTo: currentLogFile)
try fileHandle.seekToEnd()
guard let newLineData = "newline".data(using: .utf8) else {
XCTFail("Couldn't create data to write to disk.")
return
}
try fileHandle.write(contentsOf: newLineData)
try fileHandle.close()
// Then that file should now be the first log file.
XCTAssertEqual(MXLogger.logFiles.map(\.lastPathComponent),
["console.1.log",
"console-nse.5.log",
"console-nse.4.log",
"console-nse.3.log",
"console-nse.2.log",
"console-nse.1.log",
"console.5.log",
"console.4.log",
"console.3.log",
"console.2.log"])
}
func testRoomSummaryContentIsRedacted() throws {
// Given a room summary that contains sensitive information
let roomName = "Private Conversation"

View File

@ -0,0 +1 @@
Improve bug report uploads with file size checks and better error handling.