RUMM-2171 Use core feature scope in Logger #881

maxep · 2022-06-03T11:59:16Z

What and why?

Logger instances now build and record events using V1FeatureScope provided by DatadogV1CoreProtocol.

How?

The Logger now keeps a reference to the provided core to retrieve the scope when a log event needs to be recorded. The LogEventBuilder is created using the context provided with the scope, the event is then written to the writer provided by core, and an additional log output if available. The additional output allow to write the event to the console and/or to RUM Error Integration.

The userLogger static instance is now instantiated using the core created at Datadog.initialize().

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 · 2022-06-03T13:31:55Z

Sources/Datadog/Utils/InternalLoggers.swift

    )
 }

 internal func createSDKUserLogger(
-    configuration: InternalLoggerConfiguration,
+    in core: DatadogCoreProtocol,


The userLogger now needs a core instance with a registered LoggingFeature instance :/
An alternative would be to provide 2 different implementations of a Logger, one that uses core, and one that write logs directly to the console without core. LMKWYT!

Makes sense to have separate implementation for core logger. After all, we will need something self-contained that will live solely in Core module, so we can't use Logger nor even LogOutput in there. I think that a thin CoreLogger construct can make sense here, with not much "outputs" abstraction and a simple print() at at the bottom 👌.

Also, a question is: should the core logger be specific to an instance of the core or can we have a single core logger for all SDK instances? I think the prior makes more sense, as messages could be annotated with the SDK instance name. Otherwise, internal logs could be ambiguous.

Agreed, we should get a console print per core 👍

ncreated

It looks great at the high level 🎯👌. I agree that we have a problem with internal loggers now. I'd vote for solving it by doing more refactoring in this PR instead of adding bool flags in Logger.init(). I left some details in comments, LTMWDYT 🙌.

ncreated · 2022-06-06T11:20:07Z

Sources/Datadog/DatadogCore/DatadogCore.swift

+    let storage: FeatureStorage
+    let telemetry: Telemetry?
+
+    func execute(_ block: (DatadogV1Context, Writer) throws -> Void) {


This piece is called "event write context" in the original RFC to highlight the purpose of this method: obtaining the context for writing events in V2 (getting async SDK context + writer). Here we call it execute which is quite vague given that this method cannot (and shouldn't) be used for anything else. What's your take on this @maxep?

execute by itself doesn't mean much I agree, but as the single method of a protocol called FeatureScope it describes what it does: core.scope(...).execute { } -> it execute the block in the feature's scope
We could use something more explicit like executeEventWriteContext but it looks long and redundant from a usage perspective, no?

The get prefix, as proposed in the RFC, should be reserved to method that return something IMO.

this method cannot (and shouldn't) be used for anything else

Shouldn't we enforce this by design rather than convention? By defining a method that expect an event that is then written to the storage:

func buildEvent<T>(_ block: (DatadogV1Context) throws -> T?) where T: Encodable

the returned event would be then passed to the writer.

I see - indeed it makes more sense if thinking of "scope" + "execute" rather than "execute" itself 👍. I'm only worried that we won't stay in sync with Android SDK. One of the goals in the RFC was to embrace a common language, hence we introduced the concept of "event write context" that is a central point to V2 storage architecture. I'm OK with changing it, but we should try to preserve common lingo between both SDKs, as it will make discussion and plannings easier.

func buildEvent<T>(_ block: (DatadogV1Context) throws -> T?) where T: Encodable

Mind that soon we will enhance the "event write context" with metadata concept and the writer interface will become more specialised:

// ref. RFC: internal struct BatchWriter<Event: Codable, Metadata: Codable> { let currentMetadata: Metadata func write(event: Event, newMetadata: Metadata) { /* ... */ } }

so I don't think that buildEvent<T> will be able to support this. I think that starting with convention might be good for the beginning - we can change it later when we understand more. WDYT @maxep?

Indeed, I forgot about the metadata :) I will rename the method to eventWriteContext to stay aligned with Android for now, we can continue discussing naming as we move forward with the transition.

Sources/Datadog/DatadogInternal/DatadogV1CoreProtocol.swift

ncreated · 2022-06-06T11:45:17Z

Sources/Datadog/Logger.swift

-    internal let logOutput: LogOutput?
-    /// Provides date for log creation.
-    private let dateProvider: DateProvider
+    internal typealias LogEventValidation = (LogEvent) -> Bool


This looks very odd without additional context and I think it's only because of internal SDK-logger (?). If so, I'd vote on refactoring userLogger as part of this PR. In V2 we won't be able to use real Logger to print internal debug messages, so such change will be very much aligned with V2 migration. Also, we don't really leverage Logger capabilities in userLogger, so replacing it with simpler construct should be quite easy and not much impactful. Last, it will simplify this PR a bit. WDYT @maxep ?

I'm preparing for rebasing on #867 which allow filtering logs levels in dd logger as well, so this closure will still be necessary.
I'm planning on re-writing the console log, but I would tackle that in a dedicated PR so we can discuss its design. How does it sounds?

Sounds good 👍. We can consolidate this validation later, after we see the whole picture 👌.

I've added a JIRA ticket: RUMM-2239

Sources/Datadog/Logging/LogOutputs/LogFileOutput.swift

ncreated · 2022-06-06T11:53:14Z

Sources/Datadog/Logger.swift

-    private let dateProvider: DateProvider
+    internal typealias LogEventValidation = (LogEvent) -> Bool
+
+    internal let core: DatadogCoreProtocol


I don't have yet opinion on this, but should we capture strong reference to core in Features @maxep ?

By making it weak we can anticipate capability to tear down the SDK instance making its Features disabled. On the other hand, if choosing weak we might need to provide additional thread-safety measures for accessing core from Feature's thread (so it can be deallocated at any time in atomic fashion). Maybe it's better to start with strong reference.

That's a very good question! IMO, the core should exist as long as the feature exists, so strong keeps a consistent behavior.

Also, the DatadogCoreProtocol is not a class-only protocol by inheriting from AnyObject. Maybe that is something we should add, so Features can decide if core should be kept!

Let's keep it strong now and we will see when migrating other features if there's a use case for weak. There is no plan yet for "SDK can be deinitialized", so strong reference can be considered simpler solution for now 👍.

ncreated · 2022-06-06T13:16:37Z

Sources/Datadog/Utils/InternalLoggers.swift

    )
 }

 internal func createSDKUserLogger(
-    configuration: InternalLoggerConfiguration,
+    in core: DatadogCoreProtocol,


Makes sense to have separate implementation for core logger. After all, we will need something self-contained that will live solely in Core module, so we can't use Logger nor even LogOutput in there. I think that a thin CoreLogger construct can make sense here, with not much "outputs" abstraction and a simple print() at at the bottom 👌.

Co-authored-by: Maciek Grzybowski <[email protected]>

ncreated

All good 🏅

RUMM-2171 Use core feature scope in Logger

0bf046c

maxep self-assigned this Jun 3, 2022

maxep marked this pull request as ready for review June 3, 2022 12:31

maxep requested a review from a team as a code owner June 3, 2022 12:31

maxep commented Jun 3, 2022

View reviewed changes

ncreated reviewed Jun 6, 2022

View reviewed changes

maxep and others added 3 commits June 7, 2022 13:34

RUMM-2171 Update ref documentation

42d747b

Co-authored-by: Maciek Grzybowski <[email protected]>

RUMM-2171 Rename feature scope method

da6a165

RUMM-2171 Move LoggingWithRUMErrorsIntegration extension

a565033

maxep requested a review from ncreated June 8, 2022 11:27

ncreated approved these changes Jun 9, 2022

View reviewed changes

maxep merged commit ea3aaaf into feature/v2 Jun 9, 2022

maxep deleted the maxep/RUMM-2171/logging-feature-scope branch June 9, 2022 08:31

ncreated mentioned this pull request Jul 11, 2022

RUMM-2239 Fix Logger regressions and split Logger interface #923

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-2171 Use core feature scope in Logger #881

RUMM-2171 Use core feature scope in Logger #881

maxep commented Jun 3, 2022 •

edited

Loading

maxep Jun 3, 2022 •

edited

Loading

ncreated Jun 6, 2022

ncreated Jun 6, 2022

maxep Jun 7, 2022

ncreated left a comment

ncreated Jun 6, 2022

maxep Jun 7, 2022

ncreated Jun 8, 2022

maxep Jun 8, 2022

ncreated Jun 6, 2022

maxep Jun 7, 2022

ncreated Jun 7, 2022

maxep Jun 7, 2022

ncreated Jun 6, 2022

maxep Jun 7, 2022

ncreated Jun 7, 2022

ncreated Jun 6, 2022

ncreated left a comment

RUMM-2171 Use core feature scope in Logger #881

RUMM-2171 Use core feature scope in Logger #881

Conversation

maxep commented Jun 3, 2022 • edited Loading

What and why?

How?

Review checklist

Custom CI job configuration (optional)

maxep Jun 3, 2022 • 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

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

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

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Jun 3, 2022 •

edited

Loading

maxep Jun 3, 2022 •

edited

Loading