From 00fd8f7366e80cb2fed92bcc883f41026cdaed9e Mon Sep 17 00:00:00 2001 From: Jeff Ward Date: Fri, 12 Apr 2024 16:14:47 -0400 Subject: [PATCH] Add test and fix issues in log binary image attachement --- .../Tests/Datadog/Mocks/LogsMocks.swift | 6 +- .../Models/CrashReporting/BinaryImage.swift | 2 + DatadogLogs/Sources/Feature/LogsFeature.swift | 12 +++- DatadogLogs/Sources/Log/LogEventBuilder.swift | 3 +- DatadogLogs/Sources/Log/LogEventEncoder.swift | 2 +- DatadogLogs/Sources/RemoteLogger.swift | 20 ++++-- .../Tests/Mocks/LoggingFeatureMocks.swift | 6 +- DatadogLogs/Tests/RemoteLoggerTests.swift | 32 +++++++++ .../Scopes/RUMScopeDependencies.swift | 2 + .../RUMMonitor/Scopes/RUMViewScope.swift | 4 +- .../RUMMonitor/Scopes/RUMViewScopeTests.swift | 68 +++++++++++++++++++ 11 files changed, 141 insertions(+), 16 deletions(-) diff --git a/DatadogCore/Tests/Datadog/Mocks/LogsMocks.swift b/DatadogCore/Tests/Datadog/Mocks/LogsMocks.swift index 080a7e8bd6..0ef6a290df 100644 --- a/DatadogCore/Tests/Datadog/Mocks/LogsMocks.swift +++ b/DatadogCore/Tests/Datadog/Mocks/LogsMocks.swift @@ -26,13 +26,15 @@ extension LogsFeature { logEventMapper: LogEventMapper? = nil, requestBuilder: FeatureRequestBuilder = RequestBuilder(), messageReceiver: FeatureMessageReceiver = NOPFeatureMessageReceiver(), - dateProvider: DateProvider = SystemDateProvider() + dateProvider: DateProvider = SystemDateProvider(), + backtraceReporter: BacktraceReporting? = nil ) -> Self { return .init( logEventMapper: logEventMapper, requestBuilder: requestBuilder, messageReceiver: messageReceiver, - dateProvider: dateProvider + dateProvider: dateProvider, + backtraceReporter: backtraceReporter ) } } diff --git a/DatadogInternal/Sources/Models/CrashReporting/BinaryImage.swift b/DatadogInternal/Sources/Models/CrashReporting/BinaryImage.swift index e22ef97166..7d53e3be5d 100644 --- a/DatadogInternal/Sources/Models/CrashReporting/BinaryImage.swift +++ b/DatadogInternal/Sources/Models/CrashReporting/BinaryImage.swift @@ -42,3 +42,5 @@ public struct BinaryImage: Codable, PassthroughAnyCodable { case maxAddress = "max_address" } } + +extension BinaryImage: Equatable {} diff --git a/DatadogLogs/Sources/Feature/LogsFeature.swift b/DatadogLogs/Sources/Feature/LogsFeature.swift index 5e5ab84279..ffd1fa1794 100644 --- a/DatadogLogs/Sources/Feature/LogsFeature.swift +++ b/DatadogLogs/Sources/Feature/LogsFeature.swift @@ -16,6 +16,8 @@ internal struct LogsFeature: DatadogRemoteFeature { let logEventMapper: LogEventMapper? + let backtraceReporter: BacktraceReporting? + @ReadWriteLock private var attributes: [String: Encodable] = [:] @@ -26,7 +28,8 @@ internal struct LogsFeature: DatadogRemoteFeature { logEventMapper: LogEventMapper?, dateProvider: DateProvider, customIntakeURL: URL? = nil, - telemetry: Telemetry = NOPTelemetry() + telemetry: Telemetry = NOPTelemetry(), + backtraceReporter: BacktraceReporting? = nil ) { self.init( logEventMapper: logEventMapper, @@ -39,7 +42,8 @@ internal struct LogsFeature: DatadogRemoteFeature { CrashLogReceiver(dateProvider: dateProvider, logEventMapper: logEventMapper), WebViewLogReceiver() ), - dateProvider: dateProvider + dateProvider: dateProvider, + backtraceReporter: backtraceReporter ) } @@ -47,12 +51,14 @@ internal struct LogsFeature: DatadogRemoteFeature { logEventMapper: LogEventMapper?, requestBuilder: FeatureRequestBuilder, messageReceiver: FeatureMessageReceiver, - dateProvider: DateProvider + dateProvider: DateProvider, + backtraceReporter: BacktraceReporting? ) { self.logEventMapper = logEventMapper self.requestBuilder = requestBuilder self.messageReceiver = messageReceiver self.dateProvider = dateProvider + self.backtraceReporter = backtraceReporter } internal func addAttribute(forKey key: AttributeKey, value: AttributeValue) { diff --git a/DatadogLogs/Sources/Log/LogEventBuilder.swift b/DatadogLogs/Sources/Log/LogEventBuilder.swift index 89bd6a72c2..0d1361df0e 100644 --- a/DatadogLogs/Sources/Log/LogEventBuilder.swift +++ b/DatadogLogs/Sources/Log/LogEventBuilder.swift @@ -67,7 +67,8 @@ internal struct LogEventBuilder { message: $0.message, stack: $0.stack, sourceType: $0.sourceType, - fingerprint: errorFingerprint + fingerprint: errorFingerprint, + binaryImages: binaryImages ) }, serviceName: service, diff --git a/DatadogLogs/Sources/Log/LogEventEncoder.swift b/DatadogLogs/Sources/Log/LogEventEncoder.swift index 0d1942e392..1db44a5459 100644 --- a/DatadogLogs/Sources/Log/LogEventEncoder.swift +++ b/DatadogLogs/Sources/Log/LogEventEncoder.swift @@ -241,7 +241,7 @@ internal struct LogEventEncoder { try container.encode(someError.sourceType, forKey: .errorSourceType) try container.encode(someError.fingerprint, forKey: .errorFingerprint) if let binaryImages = someError.binaryImages { - try container.encode(someError.binaryImages, forKey: .errorBinaryImages) + try container.encode(binaryImages, forKey: .errorBinaryImages) } } diff --git a/DatadogLogs/Sources/RemoteLogger.swift b/DatadogLogs/Sources/RemoteLogger.swift index d76e82f526..035a9b15ee 100644 --- a/DatadogLogs/Sources/RemoteLogger.swift +++ b/DatadogLogs/Sources/RemoteLogger.swift @@ -100,7 +100,9 @@ internal final class RemoteLogger: LoggerProtocol { return } - let globalAttributes = self.core?.get(feature: LogsFeature.self)?.getAttributes() + let logsFeature = self.core?.get(feature: LogsFeature.self) + + let globalAttributes = logsFeature?.getAttributes() // on user thread: let date = dateProvider.now @@ -111,6 +113,7 @@ internal final class RemoteLogger: LoggerProtocol { var logAttributes = attributes let isCrash = logAttributes?.removeValue(forKey: CrossPlatformAttributes.errorLogIsCrash) as? Bool ?? false let errorFingerprint = logAttributes?.removeValue(forKey: Logs.Attributes.errorFingerprint) as? String + let addBinaryImages = logAttributes?.removeValue(forKey: CrossPlatformAttributes.includeBinaryImages) as? Bool ?? false let userAttributes = self.attributes .merging(logAttributes ?? [:]) { $1 } // prefer message attributes let combinedAttributes: [String: any Encodable] @@ -153,9 +156,9 @@ internal final class RemoteLogger: LoggerProtocol { // When binary images are requested, add them var binaryImages: [BinaryImage]? - if let addBinaryImages = logAttributes?.removeValue(forKey: CrossPlatformAttributes.includeBinaryImages) { - // Don't try to get binary images if we already have them. - binaryImages = try? self.core?.backtraceReporter.generateBacktrace()?.binaryImages + if addBinaryImages { + // TODO: RUM-4072 Replace full backtrace reporter with simpler binary image fetcher + binaryImages = try? logsFeature?.backtraceReporter?.generateBacktrace()?.binaryImages } let builder = LogEventBuilder( @@ -186,6 +189,13 @@ internal final class RemoteLogger: LoggerProtocol { return } + // Bit of a hack - add the `includeBinaryImages` attribute back in if it was in the original log + // TODO: RUM-4072 This double generates a backtrace - this should be optimized to use cached binary images instead + var rumAttributes: [String: Encodable] = combinedAttributes + if addBinaryImages { + rumAttributes[CrossPlatformAttributes.includeBinaryImages] = true + } + self.core?.send( message: .baggage( key: ErrorMessage.key, @@ -193,7 +203,7 @@ internal final class RemoteLogger: LoggerProtocol { message: log.error?.message ?? log.message, type: log.error?.kind, stack: log.error?.stack, - attributes: .init(combinedAttributes) + attributes: .init(rumAttributes) ) ) ) diff --git a/DatadogLogs/Tests/Mocks/LoggingFeatureMocks.swift b/DatadogLogs/Tests/Mocks/LoggingFeatureMocks.swift index 865fb5fb31..8040d4f5e3 100644 --- a/DatadogLogs/Tests/Mocks/LoggingFeatureMocks.swift +++ b/DatadogLogs/Tests/Mocks/LoggingFeatureMocks.swift @@ -42,13 +42,15 @@ extension LogsFeature { sampler: Sampler = .mockKeepAll(), requestBuilder: FeatureRequestBuilder = RequestBuilder(), messageReceiver: FeatureMessageReceiver = NOPFeatureMessageReceiver(), - dateProvider: DateProvider = SystemDateProvider() + dateProvider: DateProvider = SystemDateProvider(), + backtraceReporter: BacktraceReporting = BacktraceReporterMock(backtrace: nil) ) -> Self { return .init( logEventMapper: logEventMapper, requestBuilder: requestBuilder, messageReceiver: messageReceiver, - dateProvider: dateProvider + dateProvider: dateProvider, + backtraceReporter: backtraceReporter ) } } diff --git a/DatadogLogs/Tests/RemoteLoggerTests.swift b/DatadogLogs/Tests/RemoteLoggerTests.swift index b799049e16..7456d6f2e6 100644 --- a/DatadogLogs/Tests/RemoteLoggerTests.swift +++ b/DatadogLogs/Tests/RemoteLoggerTests.swift @@ -260,6 +260,38 @@ class RemoteLoggerTests: XCTestCase { XCTAssertEqual(log.error?.fingerprint, randomErrorFingerprint) } + func testWhenAttributesContainIncludeBinaryImages_itAddsBinaryImagesToLogEvent() throws { + let stubBacktrace: BacktraceReport = .mockRandom() + let logsFeature = LogsFeature.mockWith( + backtraceReporter: BacktraceReporterMock(backtrace: stubBacktrace) + ) + let core = SingleFeatureCoreMock( + feature: logsFeature, + expectation: expectation(description: "Send log") + ) + let logger = RemoteLogger( + core: core, + configuration: .mockAny(), + dateProvider: RelativeDateProvider(), + rumContextIntegration: false, + activeSpanIntegration: false + ) + + // When + logger.error("Information message", error: ErrorMock(), attributes: [CrossPlatformAttributes.includeBinaryImages: true]) + + // Then + waitForExpectations(timeout: 0.5, handler: nil) + + let logs = core.events(ofType: LogEvent.self) + XCTAssertEqual(logs.count, 1) + + let log = try XCTUnwrap(logs.first) + XCTAssertNil(log.attributes.userAttributes[CrossPlatformAttributes.includeBinaryImages]) + XCTAssertNotNil(log.error?.binaryImages) + XCTAssertEqual(log.error?.binaryImages, stubBacktrace.binaryImages) + } + // MARK: - RUM Integration func testWhenRUMIntegrationIsEnabled_itSendsLogWithRUMContext() throws { diff --git a/DatadogRUM/Sources/RUMMonitor/Scopes/RUMScopeDependencies.swift b/DatadogRUM/Sources/RUMMonitor/Scopes/RUMScopeDependencies.swift index 5f5c5c3637..38add82707 100644 --- a/DatadogRUM/Sources/RUMMonitor/Scopes/RUMScopeDependencies.swift +++ b/DatadogRUM/Sources/RUMMonitor/Scopes/RUMScopeDependencies.swift @@ -58,6 +58,7 @@ internal struct RUMScopeDependencies { firstPartyHosts: FirstPartyHosts?, eventBuilder: RUMEventBuilder, rumUUIDGenerator: RUMUUIDGenerator, + backtraceReporter: BacktraceReporting?, ciTest: RUMCITest?, syntheticsTest: RUMSyntheticsTest?, vitalsReaders: VitalsReaders?, @@ -72,6 +73,7 @@ internal struct RUMScopeDependencies { self.firstPartyHosts = firstPartyHosts self.eventBuilder = eventBuilder self.rumUUIDGenerator = rumUUIDGenerator + self.backtraceReporter = backtraceReporter self.ciTest = ciTest self.syntheticsTest = syntheticsTest self.vitalsReaders = vitalsReaders diff --git a/DatadogRUM/Sources/RUMMonitor/Scopes/RUMViewScope.swift b/DatadogRUM/Sources/RUMMonitor/Scopes/RUMViewScope.swift index caef19bc8f..3bc9eb304a 100644 --- a/DatadogRUM/Sources/RUMMonitor/Scopes/RUMViewScope.swift +++ b/DatadogRUM/Sources/RUMMonitor/Scopes/RUMViewScope.swift @@ -554,10 +554,10 @@ internal class RUMViewScope: RUMScope, RUMContextProvider { let errorFingerprint = commandAttributes.removeValue(forKey: RUM.Attributes.errorFingerprint) as? String var binaryImages = command.binaryImages?.compactMap { $0.toRUMDataFormat } - if let addBinaryImages = commandAttributes.removeValue(forKey: CrossPlatformAttributes.includeBinaryImages) { + if commandAttributes.removeValue(forKey: CrossPlatformAttributes.includeBinaryImages) != nil { // Don't try to get binary images if we already have them. if binaryImages == nil { - // TODO: RUM-000 Replace full backtrace reporter with simpler binary image fetcher + // TODO: RUM-4072 Replace full backtrace reporter with simpler binary image fetcher binaryImages = try? dependencies.backtraceReporter?.generateBacktrace()?.binaryImages.compactMap { $0.toRUMDataFormat } } } diff --git a/DatadogRUM/Tests/RUMMonitor/Scopes/RUMViewScopeTests.swift b/DatadogRUM/Tests/RUMMonitor/Scopes/RUMViewScopeTests.swift index 0634533f61..a55bd179bd 100644 --- a/DatadogRUM/Tests/RUMMonitor/Scopes/RUMViewScopeTests.swift +++ b/DatadogRUM/Tests/RUMMonitor/Scopes/RUMViewScopeTests.swift @@ -1557,6 +1557,74 @@ class RUMViewScopeTests: XCTestCase { XCTAssertNil(error.context!.contextInfo[RUM.Attributes.errorFingerprint]) } + func testGivenStartedView_whenErrorWithIncludeBinaryImagesAttributesIsAdded_itAddsBinaryImagesToError() throws { + // Given + let mockBacktrace: BacktraceReport = .mockRandom() + let hasReplay: Bool = .mockRandom() + var context = self.context + context.baggages = try .mockSessionReplayAttributes(hasReplay: hasReplay) + var currentTime: Date = .mockDecember15th2019At10AMUTC() + + let scope = RUMViewScope( + isInitialView: .mockRandom(), + parent: parent, + dependencies: .mockWith( + backtraceReporter: BacktraceReporterMock(backtrace: mockBacktrace) + ), + identity: .mockViewIdentifier(), + path: "UIViewController", + name: "ViewName", + attributes: [:], + customTimings: [:], + startTime: currentTime, + serverTimeOffset: .zero + ) + + XCTAssertTrue( + scope.process( + command: RUMStartViewCommand.mockWith(time: currentTime, attributes: [:], identity: .mockViewIdentifier()), + context: context, + writer: writer + ) + ) + + currentTime.addTimeInterval(1) + + // When + XCTAssertTrue( + scope.process( + command: RUMAddCurrentViewErrorCommand.mockWithErrorMessage( + time: currentTime, + message: "view error", + source: .source, + stack: nil, + attributes: [ + CrossPlatformAttributes.includeBinaryImages: true + ] + ), + context: context, + writer: writer + ) + ) + + // Then + let error = try XCTUnwrap(writer.events(ofType: RUMErrorEvent.self).last) + XCTAssertNil(error.context?.contextInfo[CrossPlatformAttributes.includeBinaryImages]) + XCTAssertNotNil(error.error.binaryImages) + XCTAssertEqual(error.error.binaryImages?.count, mockBacktrace.binaryImages.count) + for i in 0..