From f384e229b18bf8e0ad2f4db037893312ed5d805d Mon Sep 17 00:00:00 2001 From: Xavier Gouchet Date: Fri, 27 May 2022 12:53:51 +0200 Subject: [PATCH] RUMM-2173 fix code review comments --- ...CrashReportingWithLoggingIntegration.swift | 3 +- .../LoggingForTracingAdapter.swift | 3 +- Sources/Datadog/Logger.swift | 30 +++++++++++----- .../Logging/LogOutputs/LogFileOutput.swift | 7 ---- .../Datadog/LoggerBuilderTests.swift | 36 +++++++++++++------ .../LogOutputs/LogFileOutputTests.swift | 31 ++++++++-------- docs/log_collection.md | 2 +- tools/lint/run-linter.sh | 4 +-- 8 files changed, 68 insertions(+), 48 deletions(-) diff --git a/Sources/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithLoggingIntegration.swift b/Sources/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithLoggingIntegration.swift index 629c9e9d94..92476196a0 100644 --- a/Sources/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithLoggingIntegration.swift +++ b/Sources/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithLoggingIntegration.swift @@ -28,8 +28,7 @@ internal struct CrashReportingWithLoggingIntegration: CrashReportingIntegration fileWriter: loggingFeature.storage.arbitraryAuthorizedWriter, // The RUM Errors integration is not set for this instance of the `LogFileOutput` we don't want to // issue additional RUM Errors for crash reports. Those are send through `CrashReportingWithRUMIntegration`. - rumErrorsIntegration: nil, - reportingThreshold: .debug + rumErrorsIntegration: nil ), dateProvider: loggingFeature.dateProvider, dateCorrector: loggingFeature.dateCorrector, diff --git a/Sources/Datadog/FeaturesIntegration/LoggingForTracingAdapter.swift b/Sources/Datadog/FeaturesIntegration/LoggingForTracingAdapter.swift index fdfffe96ad..50fafc1596 100644 --- a/Sources/Datadog/FeaturesIntegration/LoggingForTracingAdapter.swift +++ b/Sources/Datadog/FeaturesIntegration/LoggingForTracingAdapter.swift @@ -36,8 +36,7 @@ internal struct LoggingForTracingAdapter { // The RUM Errors integration is not set for this instance of the `LogFileOutput`, as RUM Errors for // spans are managed through more comprehensive `TracingWithRUMErrorsIntegration`. // Having additional integration here would produce duplicated RUM Errors for span errors set through `span.log()` API. - rumErrorsIntegration: nil, - reportingThreshold: .debug + rumErrorsIntegration: nil ) ) } diff --git a/Sources/Datadog/Logger.swift b/Sources/Datadog/Logger.swift index c5e5482610..2b4cc978c8 100644 --- a/Sources/Datadog/Logger.swift +++ b/Sources/Datadog/Logger.swift @@ -359,7 +359,7 @@ public class Logger { return self } - /// set the minim log level reported to Datadog servers. + /// Set the minim log level reported to Datadog servers. /// Any log with a level equal or above to the threshold will be sent /// - Parameter datadogReportingThreshold: `LogLevel.debug` by default public func set(datadogReportingThreshold: LogLevel) -> Builder { @@ -446,10 +446,12 @@ public class Logger { case (true, let format?): let logOutput = CombinedLogOutput( combine: [ - LogFileOutput( - fileWriter: loggingFeature.storage.writer, - rumErrorsIntegration: LoggingWithRUMErrorsIntegration(), - reportingThreshold: self.datadogReportingThreshold + ConditionalLogOutput( + conditionedOutput: LogFileOutput( + fileWriter: loggingFeature.storage.writer, + rumErrorsIntegration: LoggingWithRUMErrorsIntegration() + ), + condition: reportLogsAbove(threshold: datadogReportingThreshold) ), LogConsoleOutput( format: format, @@ -459,10 +461,12 @@ public class Logger { ) return (logBuilder, logOutput) case (true, nil): - let logOutput = LogFileOutput( - fileWriter: loggingFeature.storage.writer, - rumErrorsIntegration: LoggingWithRUMErrorsIntegration(), - reportingThreshold: self.datadogReportingThreshold + let logOutput = ConditionalLogOutput( + conditionedOutput: LogFileOutput( + fileWriter: loggingFeature.storage.writer, + rumErrorsIntegration: LoggingWithRUMErrorsIntegration() + ), + condition: reportLogsAbove(threshold: datadogReportingThreshold) ) return (logBuilder, logOutput) case (false, let format?): @@ -481,3 +485,11 @@ public class Logger { } } } + +func reportLogsAbove(threshold: LogLevel?) -> (LogEvent) -> Bool { + return { log in + let logSeverity = LogLevel(from: log.status)?.rawValue ?? 0 + let thresholdValue = threshold?.rawValue ?? .max + return logSeverity >= thresholdValue + } +} diff --git a/Sources/Datadog/Logging/LogOutputs/LogFileOutput.swift b/Sources/Datadog/Logging/LogOutputs/LogFileOutput.swift index b183f679b1..ba5912298e 100644 --- a/Sources/Datadog/Logging/LogOutputs/LogFileOutput.swift +++ b/Sources/Datadog/Logging/LogOutputs/LogFileOutput.swift @@ -11,15 +11,8 @@ internal struct LogFileOutput: LogOutput { let fileWriter: Writer /// Integration with RUM Errors. let rumErrorsIntegration: LoggingWithRUMErrorsIntegration? - // Minimum Log Level which can be sent - let reportingThreshold: LogLevel func write(log: LogEvent) { - let logLevel: Int = LogLevel(from: log.status)?.rawValue ?? 0 - if logLevel < reportingThreshold.rawValue { - return - } - fileWriter.write(value: log) if log.status == .error || log.status == .critical { diff --git a/Tests/DatadogTests/Datadog/LoggerBuilderTests.swift b/Tests/DatadogTests/Datadog/LoggerBuilderTests.swift index ced6e20706..6a7f677ebb 100644 --- a/Tests/DatadogTests/Datadog/LoggerBuilderTests.swift +++ b/Tests/DatadogTests/Datadog/LoggerBuilderTests.swift @@ -46,8 +46,13 @@ class LoggerBuilderTests: XCTestCase { let feature = LoggingFeature.instance! XCTAssertTrue( - logger.logOutput is LogFileOutput, - "When Logging feature is enabled the Logger should use `LogFileOutput`." + logger.logOutput is ConditionalLogOutput, + "When Logging feature is enabled the Logger should use `ConditionalLogOutput`." + ) + let conditionedOutput = (logger.logOutput as! ConditionalLogOutput).conditionedOutput + XCTAssertTrue( + conditionedOutput is LogFileOutput, + "When Logging feature is enabled the Logger should use `ConditionalLogOutput` with a `LogFileOutput`." ) let logBuilder = try XCTUnwrap( logger.logBuilder, @@ -105,8 +110,13 @@ class LoggerBuilderTests: XCTestCase { let feature = LoggingFeature.instance! XCTAssertTrue( - logger.logOutput is LogFileOutput, - "When Logging feature is enabled the Logger should use `LogFileOutput`." + logger.logOutput is ConditionalLogOutput, + "When Logging feature is enabled the Logger should use `ConditionalLogOutput`." + ) + let conditionedOutput = (logger.logOutput as! ConditionalLogOutput).conditionedOutput + XCTAssertTrue( + conditionedOutput is LogFileOutput, + "When Logging feature is enabled the Logger should use `ConditionalLogOutput` with a `LogFileOutput`." ) let logBuilder = try XCTUnwrap( logger.logBuilder, @@ -127,11 +137,13 @@ class LoggerBuilderTests: XCTestCase { logger = Logger.builder.build() XCTAssertNotNil(logger.logBuilder) - XCTAssertTrue(logger.logOutput is LogFileOutput) + XCTAssertTrue(logger.logOutput is ConditionalLogOutput) + XCTAssertTrue((logger.logOutput as! ConditionalLogOutput).conditionedOutput is LogFileOutput) logger = Logger.builder.sendLogsToDatadog(true).build() XCTAssertNotNil(logger.logBuilder) - XCTAssertTrue(logger.logOutput is LogFileOutput) + XCTAssertTrue(logger.logOutput is ConditionalLogOutput) + XCTAssertTrue((logger.logOutput as! ConditionalLogOutput).conditionedOutput is LogFileOutput) logger = Logger.builder.sendLogsToDatadog(false).build() XCTAssertNil(logger.logBuilder) @@ -141,18 +153,21 @@ class LoggerBuilderTests: XCTestCase { var combinedOutputs = try (logger.logOutput as? CombinedLogOutput).unwrapOrThrow().combinedOutputs XCTAssertNotNil(logger.logBuilder) XCTAssertEqual(combinedOutputs.count, 2) - XCTAssertTrue(combinedOutputs[0] is LogFileOutput) + XCTAssertTrue(combinedOutputs[0] is ConditionalLogOutput) + XCTAssertTrue((combinedOutputs[0] as! ConditionalLogOutput).conditionedOutput is LogFileOutput) XCTAssertTrue(combinedOutputs[1] is LogConsoleOutput) logger = Logger.builder.printLogsToConsole(false).build() XCTAssertNotNil(logger.logBuilder) - XCTAssertTrue(logger.logOutput is LogFileOutput) + XCTAssertTrue(logger.logOutput is ConditionalLogOutput) + XCTAssertTrue((logger.logOutput as! ConditionalLogOutput).conditionedOutput is LogFileOutput) logger = Logger.builder.sendLogsToDatadog(true).printLogsToConsole(true).build() combinedOutputs = try (logger.logOutput as? CombinedLogOutput).unwrapOrThrow().combinedOutputs XCTAssertNotNil(logger.logBuilder) XCTAssertEqual(combinedOutputs.count, 2) - XCTAssertTrue(combinedOutputs[0] is LogFileOutput) + XCTAssertTrue(combinedOutputs[0] is ConditionalLogOutput) + XCTAssertTrue((combinedOutputs[0] as! ConditionalLogOutput).conditionedOutput is LogFileOutput) XCTAssertTrue(combinedOutputs[1] is LogConsoleOutput) logger = Logger.builder.sendLogsToDatadog(false).printLogsToConsole(true).build() @@ -161,7 +176,8 @@ class LoggerBuilderTests: XCTestCase { logger = Logger.builder.sendLogsToDatadog(true).printLogsToConsole(false).build() XCTAssertNotNil(logger.logBuilder) - XCTAssertTrue(logger.logOutput is LogFileOutput) + XCTAssertTrue(logger.logOutput is ConditionalLogOutput) + XCTAssertTrue((logger.logOutput as! ConditionalLogOutput).conditionedOutput is LogFileOutput) logger = Logger.builder.sendLogsToDatadog(false).printLogsToConsole(false).build() XCTAssertNil(logger.logBuilder) diff --git a/Tests/DatadogTests/Datadog/Logging/LogOutputs/LogFileOutputTests.swift b/Tests/DatadogTests/Datadog/Logging/LogOutputs/LogFileOutputTests.swift index 5979d6488f..be99452e4e 100644 --- a/Tests/DatadogTests/Datadog/Logging/LogOutputs/LogFileOutputTests.swift +++ b/Tests/DatadogTests/Datadog/Logging/LogOutputs/LogFileOutputTests.swift @@ -32,8 +32,7 @@ class LogFileOutputTests: XCTestCase { dateProvider: fileCreationDateProvider ) ), - rumErrorsIntegration: nil, - reportingThreshold: .debug + rumErrorsIntegration: nil ) let log1: LogEvent = .mockWith(status: .info, message: "log message 1") @@ -65,20 +64,22 @@ class LogFileOutputTests: XCTestCase { var failedCombos: [String] = [] for (thresholdLevel, logStatuses) in discardedCombos { for (logStatus) in logStatuses { - let output = LogFileOutput( - fileWriter: FileWriter( - dataFormat: LoggingFeature.dataFormat, - orchestrator: FilesOrchestrator( - directory: temporaryDirectory, - performance: PerformancePreset.combining( - storagePerformance: .writeEachObjectToNewFileAndReadAllFiles, - uploadPerformance: .noOp - ), - dateProvider: fileCreationDateProvider - ) + let output = ConditionalLogOutput( + conditionedOutput: LogFileOutput( + fileWriter: FileWriter( + dataFormat: LoggingFeature.dataFormat, + orchestrator: FilesOrchestrator( + directory: temporaryDirectory, + performance: PerformancePreset.combining( + storagePerformance: .writeEachObjectToNewFileAndReadAllFiles, + uploadPerformance: .noOp + ), + dateProvider: fileCreationDateProvider + ) + ), + rumErrorsIntegration: nil ), - rumErrorsIntegration: nil, - reportingThreshold: thresholdLevel + condition: reportLogsAbove(threshold: thresholdLevel) ) let log: LogEvent = .mockWith(status: logStatus, message: "Lorem ipsum dolor sit amet…") diff --git a/docs/log_collection.md b/docs/log_collection.md index 843ea82bf3..8ea0a9061e 100644 --- a/docs/log_collection.md +++ b/docs/log_collection.md @@ -247,7 +247,7 @@ let logger = Logger.builder ```objective-c DDLoggerBuilder *builder = [DDLogger builder]; [builder sendNetworkInfo:YES]; -[builder setWithDatadogReportingThreshold:.info] +[builder setWithDatadogReportingThreshold:.info]; [builder printLogsToConsole:YES]; DDLogger *logger = [builder build]; diff --git a/tools/lint/run-linter.sh b/tools/lint/run-linter.sh index 3bd079b64a..716ae71956 100755 --- a/tools/lint/run-linter.sh +++ b/tools/lint/run-linter.sh @@ -11,6 +11,6 @@ if [[ -z "${XCODE_VERSION_ACTUAL}" ]]; then swiftlint lint --config ./tools/lint/tests.swiftlint.yml --reporter "emoji" --strict else # when run by Xcode in Build Phase - swiftlint lint --config ./tools/lint/sources.swiftlint.yml --reporter "xcode" --fix - swiftlint lint --config ./tools/lint/tests.swiftlint.yml --reporter "xcode" --fix + swiftlint lint --config ./tools/lint/sources.swiftlint.yml --reporter "xcode" + swiftlint lint --config ./tools/lint/tests.swiftlint.yml --reporter "xcode" fi