Datadog Logs Module #1176

maxep · 2023-02-24T16:13:14Z

What and why?

Create the DatadogLogs module.

How?

I'm sorry for the size of this PR, it's partially due to the xcproj but also because I've migrated part of the unit test. Some code needed to move to DatatogInternal and TestUtilities.

`DatatogInternal` utils

The following definitions are now part of DatadogInternal

Sampler
RelativeDateProvider
DDError
ProgrammerError
InternalError
SwiftExtensions.swift

`DatadogLogger`

The DatadogLogger is now the entry point of the Logs Feature.

Configuration

The public initialise methods creates and register the Logs Feature to the given core:

public static func initialise(
    in core: DatadogCoreProtocol = defaultDatadogCore,
    uploadURL: URL,
    applicationBundleIdentifier: String,
    eventMapper: LogEventMapper? = nil,
    dateProvider: DateProvider = SystemDateProvider(),
    sampler: Sampler
) throws

This signature is not yet user-friendly and it will be revisited when we tackle configuration. Currently, only the Datadog module uses it.

Builder

The DatadogLogger also exposes a builder static variable to configure and build a logger instance. This builder pattern stays untouch but will be revisited for a more idiomatic setup.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests
Run integration tests
Run smoke tests

maxep · 2023-02-24T16:14:25Z

DatadogLogs/Sources/Builder.swift

+///         ... // customize using builder methods
+///         .build()
+///
+public class Builder {


This is exact same builder as in v1.

maxep · 2023-02-24T16:15:40Z

DatadogLogs/Sources/Feature/DatadogLogsFeature.swift

+internal let DatadogLogsFeatureName = "logging"
+
+internal struct DatadogLogsFeature: DatadogFeature {
+    let name = DatadogLogsFeatureName


I'm not happy with this, I think the name should be a static requirement.

datadog-datadog-prod-us1 · 2023-02-24T16:20:48Z

Datadog Report

Branch report: maxep/RUMM-2205/logs-feature
Commit report: 0838025

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 293 Passed, 0 Skipped, 19m 10.39s Wall Time

maxep · 2023-02-27T08:18:25Z

TestUtilities/Helpers/TestsDirectory.swift

+/// Creates `Directory` pointing to unique subfolder in `/var/folders/`.
+/// Does not create the subfolder - it must be later created with `.create()`.
+/// It returns different `Directory` each time it is called.
+public func obtainUniqueTemporaryDirectory() -> URL {


The temporary directory has been moved to TestUtilities but without the dependency to Directory or File from the v1 module. By the end of the migration I'm hoping that this logic will be used by the IntegrationTests only.

It may be also needed for large-scope tests in DatadogCore - to test data storage and end-to-end interaction with mock V2 DatadogFeature (it writes data → it uploads data). Also for small-scope tests like "file writer" or "files orchestrator".

maxep · 2023-02-27T08:19:36Z

DatadogInternal/Sources/Utils/SwiftExtensions.swift

+    /// Converts floating point value to fixed width integer with preventing overflow (and crash).
+    /// In case of overflow, the value is converted to `.min` or `.max` respectively to its sign.
+    /// - Parameter floatingPoint: the value to convert
+    public init<T: BinaryFloatingPoint>(withNoOverflow floatingPoint: T) {


This is copied from SR 👍

maxep · 2023-02-27T08:25:30Z

Tests/DatadogTests/Matchers/RUMSessionMatcher.swift

-            let firstEventTime: UInt64 = try firstEvent.jsonMatcher.value(forKeyPath: "date")
-            let secondEventTime: UInt64 = try secondEvent.jsonMatcher.value(forKeyPath: "date")
+            let firstEventTime: Int64 = try firstEvent.jsonMatcher.value(forKeyPath: "date")
+            let secondEventTime: Int64 = try secondEvent.jsonMatcher.value(forKeyPath: "date")


Dates in models are milliesec in Int64.

maxep · 2023-02-27T08:49:38Z

Sources/Datadog/Datadog.swift

-    public static var verbosityLevel: LogLevel? = nil
+    public static var verbosityLevel: CoreLoggerLevel? = nil


Note that verbosity now uses levels from core and no longer from the Log Feature.

ncreated

It looks good 👍. Due to the size of change I couldn't go deep into how we solved individual problems, but I asked some clarifying questions. Waiting for replies before approval 🙂.

ncreated · 2023-02-28T08:21:05Z

DatadogLogs/Sources/Feature/DatadogLogsFeature.swift

+import Foundation
+import DatadogInternal
+
+internal let DatadogLogsFeatureName = "logging"


nit/

Suggested change

internal let DatadogLogsFeatureName = "logging"

internal let datadogLogsFeatureName = "logging"

or even

Suggested change

internal let DatadogLogsFeatureName = "logging"

internal let logsFeatureName = "logging"

as it's internal. Maintaining "Datadog*" prefix for internal types could be tedious and misleading ("when to add it vs when not?"). Would be nice to have uniform convention.

Yes, I'm also thinking of making the Feature name a static var in the DatadogFeature protocol, so then we would do DatadogLogsFeature.name, it would also simplify registration. In the meantime, I will commit your second suggestion 👍

ncreated · 2023-02-28T08:37:58Z

DatadogLogs/Sources/Logger.swift

+///
+///     // logger reference
+///     var logger = DatadogLogger.builder.build()
+public protocol Logger {


Keep in mind this is causing #250 - do we have a plan scheduled to tackle it during V2 migration? Seperate task might be enough to make sure we don't miss it.

Yes, this Logger won't be used as a type, only DatadogLogger.

Even then, the problem will be still there:

import DatadogLogs let logger: Logger

☝️ It could be still ambiguous because Logger could either mean DatadogLogs.Logger protocol or Logger struct/enum/protocol/class defined in app module. I think there is no other way around than changing the name if we want to keep it in public interface. DDLogger could be a simple fix there - we could tackle it in separate JIRA task. WDYT?

ncreated · 2023-02-28T08:41:31Z

DatadogLogs/Sources/Scrubbing/LogEventMapper.swift

@@ -24,14 +24,14 @@ public protocol LogEventMapper {
 /// Synchronous log event mapper.
 ///
 /// The class take a flat-map closure parameter for event scrubbing
-internal final class SyncLogEventMapper: LogEventMapper {
+public final class SyncLogEventMapper: LogEventMapper {


Why it needs to be public now? In V1, the "sync" aspect of log mapper was internal 🤔💭.

Indeed, I will check!

So this class is used by the Datadog.Configuration.Builder in the Datadog module. It will be made internal when we split the config 👍

Got it 👍, please track it in JIRA 👌.

ncreated · 2023-02-28T08:46:03Z

DatadogSDK.podspec

@@ -27,5 +27,6 @@ Pod::Spec.new do |s|
                           "Sources/_Datadog_Private/include/*.h"]

  s.dependency 'DatadogInternal', s.version.to_s
+  s.dependency 'DatadogLogs', s.version.to_s


Why do we need to add this dependency for DatadogCore? If we keep adding all modules here, we won't achieve the V2 goal of selective dependency ("if I don't use DD Logs, I don't want to link it"), no?

This is not the core yet, We need that dependency for initialisation. The configuration part will be tackled separately.

Oh got it - it's transient and only for migration.

ncreated · 2023-02-28T08:50:48Z

TestUtilities/Helpers/TestsDirectory.swift

+/// Creates `Directory` pointing to unique subfolder in `/var/folders/`.
+/// Does not create the subfolder - it must be later created with `.create()`.
+/// It returns different `Directory` each time it is called.
+public func obtainUniqueTemporaryDirectory() -> URL {


It may be also needed for large-scope tests in DatadogCore - to test data storage and end-to-end interaction with mock V2 DatadogFeature (it writes data → it uploads data). Also for small-scope tests like "file writer" or "files orchestrator".

maxep · 2023-02-28T10:39:55Z

Thanks for the review @ncreated and sorry again for this size. I've answered your comments, let me know if you need more clarification 👍

ncreated

Looks good 👍, thanks for answers 🙂.

maxep requested a review from a team as a code owner February 24, 2023 16:13

maxep self-assigned this Feb 24, 2023

maxep commented Feb 24, 2023

View reviewed changes

maxep commented Feb 27, 2023

View reviewed changes

maxep force-pushed the maxep/RUMM-2205/logs-feature branch 2 times, most recently from 2489928 to 48f23c2 Compare February 27, 2023 10:17

maxep added 8 commits February 27, 2023 11:31

Create DatadogLogs Module

0bd4da8

Add DatadogLogs.podspec

aa260e5

Update IntegrationTests

dc00293

Add DatadogLogs to package

dab5210

Use swift extension from internal module

091c6f0

Fix date assertion

c18baab

Update benchmark tests

6d22a61

Fix cocoapod test

a16cf4a

maxep force-pushed the maxep/RUMM-2205/logs-feature branch from 48f23c2 to a16cf4a Compare February 27, 2023 10:45

maxep added 2 commits February 27, 2023 15:33

Fix carthage test

aa9974a

Fix spm test

b1e24b6

maxep force-pushed the maxep/RUMM-2205/logs-feature branch from c72c273 to b1e24b6 Compare February 27, 2023 14:58

ncreated reviewed Feb 28, 2023

View reviewed changes

Rename DatadogLogsFeatureName to logsFeatureName

0838025

maxep requested a review from ncreated February 28, 2023 10:39

ncreated approved these changes Feb 28, 2023

View reviewed changes

maxep merged commit 3dd956f into feature/v2 Mar 1, 2023

maxep deleted the maxep/RUMM-2205/logs-feature branch March 1, 2023 08:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datadog Logs Module #1176

Datadog Logs Module #1176

maxep commented Feb 24, 2023

maxep Feb 24, 2023

maxep Feb 24, 2023

datadog-datadog-prod-us1 bot commented Feb 24, 2023 •

edited

Loading

maxep Feb 27, 2023

ncreated Feb 28, 2023

maxep Feb 27, 2023

maxep Feb 27, 2023 •

edited

Loading

maxep Feb 27, 2023

ncreated left a comment

ncreated Feb 28, 2023

maxep Feb 28, 2023

ncreated Feb 28, 2023

maxep Feb 28, 2023

ncreated Feb 28, 2023

ncreated Feb 28, 2023

maxep Feb 28, 2023

maxep Feb 28, 2023 •

edited

Loading

ncreated Feb 28, 2023

ncreated Feb 28, 2023

maxep Feb 28, 2023

ncreated Feb 28, 2023

ncreated Feb 28, 2023

maxep commented Feb 28, 2023

ncreated left a comment

		public static var verbosityLevel: LogLevel? = nil
		public static var verbosityLevel: CoreLoggerLevel? = nil

	internal let DatadogLogsFeatureName = "logging"
	internal let datadogLogsFeatureName = "logging"

	internal let DatadogLogsFeatureName = "logging"
	internal let logsFeatureName = "logging"

Datadog Logs Module #1176

Datadog Logs Module #1176

Conversation

maxep commented Feb 24, 2023

What and why?

How?

DatatogInternal utils

DatadogLogger

Configuration

Builder

Review checklist

Custom CI job configuration (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Feb 24, 2023 • edited Loading

Datadog Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep commented Feb 28, 2023

ncreated left a comment

Choose a reason for hiding this comment

`DatatogInternal` utils

`DatadogLogger`

datadog-datadog-prod-us1 bot commented Feb 24, 2023 •

edited

Loading

maxep Feb 27, 2023 •

edited

Loading

maxep Feb 28, 2023 •

edited

Loading