RUM-3294 Add the Core Tracer tests by mariusc83 · Pull Request #1924 · DataDog/dd-sdk-android

mariusc83 · 2024-03-20T13:25:46Z

What does this PR do?

In this PR we are porting the core unit tests for the Java CoreTracer logic we borrowed from the APM Agent Java project. Please have in mind that in order to have consistency those tests and the logic around are matching exactly their groovy equivalents so no randomness was used in the input data and no forgery was added.

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-20T13:46:40Z

Codecov Report

❌ Patch coverage is 12.00000% with 154 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.70%. Comparing base (50e49ba) to head (f71b1ab).

⚠️ Current head f71b1ab differs from pull request most recent head bac5936

Please upload reports for the commit bac5936 to get more accurate results.

Files with missing lines	Patch %	Lines
...m/datadog/trace/core/propagation/W3CHttpCodec.java	3.80%	151 Missing and 1 partial ⚠️
...va/com/datadog/trace/common/writer/NoOpWriter.java	66.67%	2 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/otel-support    #1924      +/-   ##
========================================================
+ Coverage                 56.46%   59.70%   +3.24%     
========================================================
  Files                       796      797       +1     
  Lines                     29589    29612      +23     
  Branches                   4852     4854       +2     
========================================================
+ Hits                      16705    17678     +973     
+ Misses                    11776    10736    -1040     
- Partials                   1108     1198      +90

Files with missing lines	Coverage Δ
...ce/src/main/java/com/datadog/trace/api/Config.java	`51.55% <100.00%> (+1.13%)`	⬆️
...java/com/datadog/trace/api/InstrumenterConfig.java	`53.78% <100.00%> (-0.84%)`	⬇️
...va/com/datadog/trace/common/writer/ListWriter.java	`54.17% <100.00%> (+54.17%)`	⬆️
...adog/trace/common/writer/TraceStructureWriter.java	`45.28% <ø> (+45.28%)`	⬆️
...c/main/java/com/datadog/trace/core/CoreTracer.java	`63.15% <100.00%> (+24.58%)`	⬆️
...e/src/main/java/com/datadog/trace/core/DDSpan.java	`56.65% <ø> (+34.33%)`	⬆️
...main/java/com/datadog/trace/core/PendingTrace.java	`80.53% <100.00%> (+32.68%)`	⬆️
...ava/com/datadog/trace/core/PendingTraceBuffer.java	`64.89% <ø> (+46.81%)`	⬆️
...va/com/datadog/trace/common/writer/NoOpWriter.java	`66.67% <66.67%> (ø)`
...m/datadog/trace/core/propagation/W3CHttpCodec.java	`3.80% <3.80%> (+0.63%)`	⬆️

... and 81 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0xnm

I gave it a quick look and added some comments. Also my concern is that some things are public now, can we review if all of them are needed to be public for the sake of the tests? Maybe it is better to access them with reflection instead of making public for everyone?

0xnm · 2024-03-25T11:49:06Z

+    policy: pull
+  script:
+    - rm -rf ~/.gradle/daemon/
+    - GRADLE_OPTS="-Xmx3072m" DD_TAGS="test.configuration.variant:release" ./gradlew :features:dd-sdk-android-trace:testDebugUnitTest --stacktrace


Suggested change

- GRADLE_OPTS="-Xmx3072m" DD_TAGS="test.configuration.variant:release" ./gradlew :features:dd-sdk-android-trace:testDebugUnitTest --stacktrace

- GRADLE_OPTS="-Xmx3072m" DD_TAGS="test.configuration.variant:debug" ./gradlew :features:dd-sdk-android-trace:testDebugUnitTest --stacktrace

0xnm · 2024-03-25T11:52:25Z

this needs to be restored

why restored ? these are api changes reflected by the changes we added in the java project to make some classes/properties available for our unit tests.

because this file is deleted completely in this PR. It should still be there with the actual Kotlin API of this module.

0xnm · 2024-03-25T11:53:54Z


    // This has to be placed after all other static fields to give them a chance to initialize

-    private static final Config INSTANCE =


why we remove this one if anyway we are going to use reflection here?

I am removing the final modifier as I can't use reflection on that anymore with JDK17 :(. Are you aware of anyway to do it in JDK17 ?

yes, we have tests which set static final on JDK 17 :)

0xnm · 2024-03-25T11:54:16Z

  }

  // This has to be placed after all other static fields to give them a chance to initialize
-  private static final InstrumenterConfig INSTANCE =


same question as above

0xnm · 2024-03-25T12:09:39Z

+    fun `should build span timestamp in nano`() {
+        // time in micro


Suggested change

fun `should build span timestamp in nano`() {

// time in micro

fun `should build span timestamp in nanoseconds`() {

// time in microseconds

0xnm · 2024-03-25T12:10:50Z

+
+        // auto-timestamp in nanoseconds
+        val start = System.currentTimeMillis()
+        span = tracer.buildSpan(instrumentationName, expectedName).withServiceName("foo").start()


Suggested change

span = tracer.buildSpan(instrumentationName, expectedName).withServiceName("foo").start()

val secondSpan = tracer.buildSpan(instrumentationName, expectedName).withServiceName("foo").start()

same suggestion applies to other tests - instead of using var and changing the reference, it is better to be more explicit in saying that it is another instance.

0xnm · 2024-03-25T12:11:29Z

+        // Give a range of +/- 5 millis
+        assertThat(span.startTime).isGreaterThanOrEqualTo(TimeUnit.MILLISECONDS.toNanos(start - 1))
+        assertThat(span.startTime).isLessThanOrEqualTo(TimeUnit.MILLISECONDS.toNanos(stop + 1))


actual offset is 1 millisecond, not 5 as it is said in the comment

yeah...I guess they modified it without modifying the comment

0xnm · 2024-03-25T12:14:16Z

+        assertThat(span.traceId).isNotEqualTo(DDTraceId.ZERO)
+        assertThat(span.parentId).isEqualTo(DDSpanId.ZERO)
+        val samplingPriority = span.samplingPriority
+        assert(samplingPriority == null)


Suggested change

assert(samplingPriority == null)

assertThat(samplingPriority).isNull()

yeah for some reason that is not working, failing with a NPE and I think is because of the Java function signature at the end of the line.

I'm not clear what do you mean. It shouldn't be any NPE here, assertThat accepts null values, so the proposed suggestion should work fine. The only assumption which comes to my mind is that type of samplingPriority is Int? and by some reason Kotlin tries to cast it to Int, but it is quite unlikely.

0xnm · 2024-03-25T12:14:54Z

+        val customTracer = tracerBuilder().writer(writer).config(Config.get()).defaultSpanTags(tags).build()
+        val span = customTracer.buildSpan(instrumentationName, "op name").withServiceName("foo").start()


it is better to wrap such lines call-per-call (and in other tests as well)

0xnm

nice work! I added few comments/suggestions, but nothing blocking.

0xnm · 2024-03-26T10:40:14Z


  KUBERNETES_MEMORY_REQUEST: "8Gi"
  KUBERNETES_MEMORY_LIMIT: "16Gi"
+  DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED: "false"


I think it is better to set this env variable only in the tasks which require it to be set (trace-debug and trace-release, probably). Not sure what it does though.

it is not affecting any other tests, it is strictly related with the tracing tests and code but yes I could add it there, I just did not want to duplicate this line.

0xnm · 2024-03-26T10:43:59Z

   * @param context the context used for the span
   */
-  private DDSpan(
+  public DDSpan(


is it possible to put VisibleForTesting annotation to the elements we are making public just for testing?

0xnm · 2024-03-26T10:45:20Z

        whenever(mockTracer.partialFlushMinSpans) doReturn 1
        val fakeTraceId = BigInteger.valueOf(forge.aLong())
-        val pendingTrace = PendingTrace(mockTracer, fakeTraceId)
+        val pendingTrace = createInstanceWithPrivateConstructor(PendingTrace::class.java, mockTracer, fakeTraceId)


which constructor is private here? we made one constructor of PendingTrace public, was it the necessary change if we are calling private constructor anyway?

yeah forgot to turn the constructor private back

0xnm · 2024-03-26T10:49:44Z

+    }
+
+    @Test
+    fun `should build span timestamp in nano`() {


Suggested change

fun `should build span timestamp in nano`() {

fun `should build span timestamp in nanoseconds`() {

0xnm · 2024-03-26T11:12:48Z

+        @Suppress("DEPRECATION")
+        span1.samplingPriority = PrioritySampling.USER_KEEP.toInt()


I see that we have 2 PrioritySampling classes:

com.datadog.trace.api.sampling.PrioritySampling

com.datadog.trace.common.sampling.PrioritySampling

The latter one is deprecated, the former one is not. Probably we need to use the former one here to avoid deprecation warning?

I will double check

In their tests they are using the same class so I would rather keep it but will double check.

well, this is essentially the same class in 2 different places of the codebase. Anyway, not sure what is the point to use the deprecated one when there is non-deprecated.

yes, I know, I will probably figure it out better later. For now I checked their tests source and they are using the same.

0xnm · 2024-03-26T11:13:10Z

+        }
+        // When


Suggested change

}

// When

}

// When

0xnm · 2024-03-26T11:15:21Z

+    @Test
+    fun `test getCurrentTimeNano`() {
+        // Generous 5 seconds to execute this test
+        assertThat(
+            Math.abs(
+                TimeUnit.NANOSECONDS.toSeconds(trace.currentTimeNano) -
+                        TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis())
+            )
+        ).isLessThan(5)
+    }


what is the exact point of this test? not quite clear

not sure, this is what they have in their test classes, I think they want to assert that the clock is not skewed

there is non-system clock in the code we are porting? is it using NTP then or what if it is the case?

I am really not sure, I really cannot without asking them and I cannot really ask all these details. We have 2 options here, decide to keep all the tests they have or we drop some.

ok, let's keep it then. If there is a NTP support, then it is probably some code around it/network stack while we have our own NTP support (so double the code for the same).

0xnm · 2024-03-26T11:21:44Z

+        params[it]?.javaClass ?: Any::class.java
+    }
+
+    val method = this.getDeclaredMethodRecursively(methodName, true, declarationParams)


it is useful to search method recursively given that static methods cannot be overridden and normally, if called directly, without reflection, IDE should give a warning if method is called on the class it doesn't belong directly?

yeah you are right it is not needed here.

0xnm

lgtm. I left few questions, but nothing blocking.

Also speaking of #1924 (comment) - should we add nullable/non-nullable annotations to the public Java API we bringing to have a better interop with Kotlin in order to avoid such things on the customer side? Or the only API exposed / advised to be used will be in Kotlin?

0xnm · 2024-03-26T15:33:16Z


 public class com/datadog/trace/core/DDSpan : com/datadog/trace/api/profiling/TransientProfilingContextHolder, com/datadog/trace/bootstrap/instrumentation/api/AgentSpan, com/datadog/trace/bootstrap/instrumentation/api/AttachableWrapper, com/datadog/trace/core/CoreSpan {
 	public static final field CHECKPOINTED_TAG Ljava/lang/String;
+	public fun <init> (Ljava/lang/String;JLcom/datadog/trace/core/DDSpanContext;Ljava/util/List;)V


minor: given that we have a method to call private constructor now, should we make this constructor public?

actually not...

I am not really sure where this is coming from as I am just exposing the class not the constructors so I will leave it as is. I need the class exposed to cast to DDSpan in the unit tests.

It is from this change https://github.com/DataDog/dd-sdk-android/pull/1924/files#diff-1c12b21f74915bf39d879d5d17374ad221d56bb67bed99743adc8a86dfbf944cR127 where private constructor was made public. But that is ok I believe, just was wondering about the consistency in the approach (since for PendingTrace we are using reflection to create an instance).

0xnm · 2024-03-26T15:36:45Z

+  stage: test
+  timeout: 1h
+  variables:
+    DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED: "false"


btw is this flag still needed given that we clean up env variable in the DDSpecification and default value of this one in Config is false?

yes I am not sure where exactly before that is still used...before the Config is even created. as you could see in my tests I assert that there is no more any DD_ property in any System or System.env and still this poses problems. Alessio from APM team recommended me to add it like this to make sure.

We can remove it and see if it is still needed. Maybe it is not anymore.

I tried, I will try again ;)

mariusc83 · 2024-03-26T16:27:09Z

lgtm. I left few questions, but nothing blocking.

Also speaking of #1924 (comment) - should we add nullable/non-nullable annotations to the public Java API we bringing to have a better interop with Kotlin in order to avoid such things on the customer side? Or the only API exposed / advised to be used will be in Kotlin?

We only want to provide Kotlin support so normally they should not touch the Java API but it might be worthy to have the discussion for any other Java support we offer, outside of Trace.

0xnm · 2024-03-26T16:37:07Z

@mariusc83 Cannot it be the case that some Java objects (like spans, etc.) are exposed in the Kotlin APIs we are going to provide?

mariusc83 · 2024-03-26T16:43:28Z

@mariusc83 Cannot it be the case that some Java objects (like spans, etc.) are exposed in the Kotlin APIs we are going to provide?

The idea about OTel support is to only use the OTel API so they should not see any Java objects in their code. I think we are safe here.

mariusc83 self-assigned this Mar 20, 2024

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch from b9eca27 to 5eb2941 Compare March 20, 2024 13:42

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch 2 times, most recently from 8e26066 to 858200e Compare March 20, 2024 14:46

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 3 times, most recently from a514f5b to c1dc25b Compare March 21, 2024 09:37

mariusc83 force-pushed the mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans branch from 858200e to dcd9317 Compare March 21, 2024 09:59

Base automatically changed from mconstantin/rum-3516/correctly-provide-sampling-tags-for-otel-spans to feature/otel-support March 21, 2024 11:09

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 2 times, most recently from fd62be7 to 2c936c4 Compare March 21, 2024 11:54

RUM-3268 Provide the Java CoreTracer unit tests

d889a92

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 17 times, most recently from 79efa36 to d607e0a Compare March 22, 2024 13:33

mariusc83 requested review from a team as code owners March 25, 2024 10:34

0xnm requested changes Mar 25, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 3 times, most recently from f7bff6f to e87032a Compare March 26, 2024 10:11

0xnm reviewed Mar 26, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 2 times, most recently from a999274 to b767ff6 Compare March 26, 2024 15:03

0xnm previously approved these changes Mar 26, 2024

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via b8b913b March 26, 2024 16:45

mariusc83 force-pushed the mconstantin/rum-3268/port-all-core-tracer-unit-tests branch 14 times, most recently from 2bc67ec to 42d50f9 Compare March 27, 2024 14:33

Fix unit tests on CI

bac5936

0xnm approved these changes Mar 27, 2024

View reviewed changes

	- GRADLE_OPTS="-Xmx3072m" DD_TAGS="test.configuration.variant:release" ./gradlew :features:dd-sdk-android-trace:testDebugUnitTest --stacktrace
	- GRADLE_OPTS="-Xmx3072m" DD_TAGS="test.configuration.variant:debug" ./gradlew :features:dd-sdk-android-trace:testDebugUnitTest --stacktrace


		// This has to be placed after all other static fields to give them a chance to initialize

		private static final Config INSTANCE =

		fun `should build span timestamp in nano`() {
		// time in micro

	span = tracer.buildSpan(instrumentationName, expectedName).withServiceName("foo").start()
	val secondSpan = tracer.buildSpan(instrumentationName, expectedName).withServiceName("foo").start()

	assert(samplingPriority == null)
	assertThat(samplingPriority).isNull()

		val customTracer = tracerBuilder().writer(writer).config(Config.get()).defaultSpanTags(tags).build()
		val span = customTracer.buildSpan(instrumentationName, "op name").withServiceName("foo").start()

		@Suppress("DEPRECATION")
		span1.samplingPriority = PrioritySampling.USER_KEEP.toInt()

-        }
-        // When
+        }
+        // When

Uh oh!

Conversation

mariusc83 commented Mar 20, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariusc83 Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 20, 2024 •

edited

Loading

mariusc83 Mar 26, 2024 •

edited

Loading