RUM-3405 Provide core tracer logger implementation #1953

mariusc83 · 2024-03-29T13:31:49Z

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2024-03-29T14:28:01Z

Codecov Report

Merging #1953 (92bd5f3) into feature/otel-support (3cfe958) will increase coverage by 0.06%.
The diff coverage is 41.79%.

❗ Current head 92bd5f3 differs from pull request most recent head ef74896. Consider uploading reports for the commit ef74896 to get more accurate results

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #1953      +/-   ##
========================================================
+ Coverage                 60.00%   60.06%   +0.06%     
========================================================
  Files                       803      805       +2     
  Lines                     30050    30119      +69     
  Branches                   4928     4929       +1     
========================================================
+ Hits                      18030    18089      +59     
- Misses                    10810    10822      +12     
+ Partials                   1210     1208       -2

Files	Coverage Δ
...c/main/java/com/datadog/trace/core/CoreTracer.java	`63.71% <100.00%> (+0.36%)`	⬆️
...e/src/main/java/com/datadog/trace/core/DDSpan.java	`56.47% <100.00%> (-0.19%)`	⬇️
...main/java/com/datadog/trace/core/PendingTrace.java	`80.95% <100.00%> (ø)`
...ava/com/datadog/trace/core/PendingTraceBuffer.java	`64.89% <100.00%> (ø)`
.../trace/core/scopemanager/AbstractContinuation.java	`100.00% <100.00%> (ø)`
...main/java/com/datadog/trace/logger/NoOpLogger.java	`33.33% <ø> (+5.21%)`	⬆️
...in/com/datadog/android/trace/OtelTracerProvider.kt	`96.05% <100.00%> (+0.05%)`	⬆️
...datadog/android/trace/logger/NoOpInternalLogger.kt	`100.00% <100.00%> (ø)`
...tadog/trace/core/scopemanager/ContinuingScope.java	`0.00% <0.00%> (ø)`
...n/java/com/datadog/trace/logger/LoggerFactory.java	`42.86% <50.00%> (+2.86%)`	⬆️
... and 6 more

... and 26 files with indirect coverage changes

0xnm · 2024-04-02T07:35:43Z

dd-sdk-android-core/src/main/kotlin/com/datadog/android/api/InternalLogger.kt

+        @JvmField
        val UNBOUND: InternalLogger = SdkInternalLogger(null)


I think you don't need to use UNBOUND. According to the code, you have access to the sdkCore.internalLogger in the OtelTracerProvider.Builder.build() which calls CoreTracer.CoreTracerBuilder() - you can just inject sdkCore.internalLogger there and pass it down to the CoreTracer instance then.

Is there a need in the UNBOUND then and thus this change?

yes, I think I did not want to provide it as optional there in the builder but yes it can work this way. I will change it back.

I'm not sure why it should be optional. If it is provided in the constructor of CoreTracer.CoreTracerBuilder() why it should be?

InternalLogger.UNBOUND is only for cases when there is no possibility to get logger associated with particular SDK core. This logger is not able to send telemetry and logs produced cannot print the SDK core instance used.

yes...that's the thing, I did not want to provide it in the constructor, not sure why, can't remember, but think it about it now I don't see why I should not.

actually it is because I do not want to use modify the static method: CoreTracer:builder to CoreTracer:builder(logger) but will provide a NoOpInternalLogger implementation instead.

0xnm

I left some comments, but I guess nothing blocking.

0xnm · 2024-04-02T15:09:21Z

features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/CoreTracer.java

+        public CoreTracerBuilder internalLogger(InternalLogger internalLogger) {
+            this.internalLogger = internalLogger;
+            return this;
+        }


is this method still needed given that we use constructor to inject logger?

I am keeping it just in case.

0xnm · 2024-04-02T15:10:37Z

features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/DDSpan.java

-      @NonNull DDSpanContext context,
-      final List<AgentSpanLink> links) {
-    final DDSpan span = new DDSpan(instrumentationName, timestampMicro, context, links);
-    log.debug("Started span: {}", span);


don't need this anymore?

I think I removed it because I could see the log also elsewhere. I will double check.

yes, it was a call on a static instance log which now it is a private class instance. I will move it to the constructor instead.

features/dd-sdk-android-trace/src/main/java/com/datadog/trace/core/DDSpan.java

0xnm · 2024-04-03T07:45:17Z

...res/dd-sdk-android-trace/src/main/java/com/datadog/trace/logger/DatadogCoreTracerLogger.java

+import java.util.Arrays;
+import java.util.Locale;
+
+public class DatadogCoreTracerLogger implements Logger {


minor thought: is name still valid? I don't see any relationship with CoreTracer here.

It is as this is the Logger implementation for the CoreTracer module.

...res/dd-sdk-android-trace/src/main/java/com/datadog/trace/logger/DatadogCoreTracerLogger.java

0xnm · 2024-04-03T07:49:24Z

features/dd-sdk-android-trace/src/main/java/com/datadog/trace/logger/LoggerFactory.java

 public final class LoggerFactory {

+    @NonNull
    public static Logger getLogger(String name) {
        return new NoOpLogger();


seems like we still can have places without any log coverage?

yes but it is quite a lot if we want to cover all the code, I covered all the important places and most of all the other places where these methods are used are not actually needed in the CoreTracer and maybe I will remove them at the end in a final clean.

0xnm · 2024-04-03T07:50:01Z

features/dd-sdk-android-trace/src/main/kotlin/com/datadog/android/trace/OtelTracerProvider.kt

                .withProperties(properties())
                .serviceName(serviceName)
                .writer(tracingFeature?.otelDataWriter ?: NoOpOtelWriter())
                .partialFlushMinSpans(partialFlushThreshold)
                .idGenerationStrategy(IdGenerationStrategy.fromName("SECURE_RANDOM", false))
+                .internalLogger(sdkCore.internalLogger)


why do we need to call this if we already injected logger in the constructor above?

I wanted to keep this method just in case for flexibility later. Not sure if we still need to. What do you think ?

if we need it, we can add it later, but currently it creates just some confusion in the API imo, since it is already provided in the constructor.

0xnm · 2024-04-03T08:00:08Z

...dd-sdk-android-trace/src/test/kotlin/com/datadog/trace/logger/DatadogCoreTracerLoggerTest.kt

+    @StringForgery
+    lateinit var fakeMessage: String
+
+    private lateinit var expectedFakeMessage: String


minor suggestion: fakes lingvo is usually just for the setup phase. And also for the consistency, because in some assertions it is already called as expectedMessage.

Another suggestion is to remove it from the properties as have as a function.

Suggested change

private lateinit var expectedFakeMessage: String

private lateinit var expectedMessage: String

Introduce the DatadogCoreTracerLogger

694c121

mariusc83 self-assigned this Mar 29, 2024

mariusc83 requested review from a team as code owners March 29, 2024 13:31

mariusc83 marked this pull request as draft March 29, 2024 13:32

mariusc83 force-pushed the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch 2 times, most recently from 2a7a9a4 to e52fa08 Compare March 29, 2024 13:53

mariusc83 marked this pull request as ready for review March 29, 2024 14:56

0xnm reviewed Apr 2, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch 3 times, most recently from 92bd5f3 to ef74896 Compare April 2, 2024 14:42

mariusc83 requested a review from 0xnm April 2, 2024 14:53

0xnm previously approved these changes Apr 3, 2024

View reviewed changes

Delegate all the logs to new DatadogCoreTracerLogger in the CoreTracer

41370c3

mariusc83 dismissed 0xnm’s stale review via 41370c3 April 3, 2024 08:34

mariusc83 force-pushed the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch from ef74896 to 41370c3 Compare April 3, 2024 08:34

mariusc83 requested a review from 0xnm April 3, 2024 08:53

0xnm approved these changes Apr 3, 2024

View reviewed changes

mariusc83 merged commit 0e43b0d into feature/otel-support Apr 3, 2024
25 checks passed

mariusc83 deleted the mconstantin/rum-3405/provide-core-tracer-logger-implementation branch April 3, 2024 09:30

xgouchet added this to the 2.11.x milestone Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-3405 Provide core tracer logger implementation #1953

RUM-3405 Provide core tracer logger implementation #1953

mariusc83 commented Mar 29, 2024

codecov-commenter commented Mar 29, 2024 •

edited

Loading

0xnm Apr 2, 2024

mariusc83 Apr 2, 2024

0xnm Apr 2, 2024

mariusc83 Apr 2, 2024

mariusc83 Apr 2, 2024

0xnm left a comment

0xnm Apr 2, 2024

mariusc83 Apr 3, 2024

0xnm Apr 2, 2024

mariusc83 Apr 3, 2024

mariusc83 Apr 3, 2024

0xnm Apr 3, 2024

mariusc83 Apr 3, 2024

0xnm Apr 3, 2024

mariusc83 Apr 3, 2024

0xnm Apr 3, 2024

mariusc83 Apr 3, 2024

0xnm Apr 3, 2024

0xnm Apr 3, 2024

		@JvmField
		val UNBOUND: InternalLogger = SdkInternalLogger(null)

	private lateinit var expectedFakeMessage: String
	private lateinit var expectedMessage: String

RUM-3405 Provide core tracer logger implementation #1953

RUM-3405 Provide core tracer logger implementation #1953

Conversation

mariusc83 commented Mar 29, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Mar 29, 2024 • edited Loading

Codecov Report

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

0xnm 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

Choose a reason for hiding this comment

codecov-commenter commented Mar 29, 2024 •

edited

Loading