RUMM-2173 fix code review comments

DataDog · May 27, 2022 · f384e22 · f384e22
1 parent 7b83ccc
commit f384e22
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 48 deletions.
diff --git a/...ces/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithLoggingIntegration.swift b/...ces/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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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