Move the adaptive sampler to a shared location. by jbachorik · Pull Request #2418 · DataDog/dd-trace-java

jbachorik · 2021-02-18T15:47:47Z

The rate limiting sampler is not specific to exception profiling only so it should live in a more generic space.

The change is rather harmless - just moving the actual sampler implementation to internal-api project. Since the agent-bootstrap already depends on internal-api no further changes are needed.

The rate limiting sampler is not specific to exception profiling only so it should live in a more generic space.

richardstartin · 2021-02-18T18:54:03Z

@@ -1,3 +1,7 @@
+ext {
+  enableJunitPlatform = true


richardstartin · 2021-02-18T18:55:02Z

+compileTestJava {
+  sourceCompatibility = 8
+  targetCompatibility = 8
+}


Does this mean we no longer test internal-api on JDK7?

richardstartin

We can't stop compiling and testing internal-api on JDK7. Perhaps extract an interface and move the implementation into a JDK8 module under utils, with a stub implementation and a factory, then load the JDK8 implementation based on a version check.

richardstartin · 2021-02-18T19:02:45Z

   */
-  StreamingSampler(
+  public AdaptiveSampler(
      final Duration windowDuration,


(Stating the obvious) Java time was introduced in JDK8, so this implementation would need to be moved out of internal-api

jbachorik · 2021-02-19T09:30:43Z

Well, my intention was to have just the one source root to be compiled with Java 8 to avoid creating a new module just for this one class. Apparently, it is not that easy even with the gradle black magic :(

BTW, with move to ASM 9 to support JDK 16 bytecode level the support for Java 7 will be gone as ASM itself is compiled to level 52 (Java 8) 🤷

jbachorik · 2021-02-19T10:39:34Z

The AdaptiveSampler implementation has been moved to a separate submodule of internal-api.

Since it is used in an already Java 8+ code in the bootstrap I would like to avoid jumping through several facade/reflection hoops in order to use it. The appropriate checks are done at the top level when the whole profiler subsystem is enabled for Java 8+.

Request met

richardstartin · 2021-02-19T12:20:42Z

BTW, with move to ASM 9 to support JDK 16 bytecode level the support for Java 7 will be gone as ASM itself is compiled to level 52 (Java 8) 🤷

Yes, I agree, maintaining support for JDK7 is holding us back, but we have users on JDK7 and can't drop it until the product decision has been made.

jbachorik · 2021-02-19T16:36:06Z

we have users on JDK7 and can't drop it until the product decision has been made

My (slightly offtopic) point was that with ASM 9 you don't have the choice any more :(

Anyway, for this PR I pulled out the Java 8 specific code to a separate module to make it more explicit (also, it makes the build much simpler).

richardstartin · 2021-02-19T16:44:10Z

My (slightly offtopic) point was that with ASM 9 you don't have the choice any more :(

I got it, and this will force the issue soon, as it presents a dichotomy between JDK7 and JDK17

richardstartin · 2021-03-02T14:54:44Z

 compileMain_java11Java.targetCompatibility = JavaVersion.VERSION_1_8
 dependencies {
-  main_java11CompileOnly project(':internal-api')
+  main_java11Compile project(':internal-api:internal-api-8')


Why compileOnly -> compile?

richardstartin · 2021-03-02T14:56:21Z

This looks problematic

java.lang.NoClassDefFoundError: datadog/trace/api/sampling/AdaptiveSampler
	at datadog.trace.bootstrap.instrumentation.exceptions.ExceptionSampler.<init>(ExceptionSampler.java:25)
	at datadog.trace.bootstrap.instrumentation.exceptions.ExceptionSampler.<init>(ExceptionSampler.java:21)
	at datadog.trace.bootstrap.instrumentation.exceptions.ExceptionProfiling.<init>(ExceptionProfiling.java:26)
	at datadog.trace.bootstrap.instrumentation.exceptions.ExceptionProfiling.<clinit>(ExceptionProfiling.java:11)
	at java.lang.Class.forName(Class.java:377)
	at AgentTestRunnerTest.classpath setup(AgentTestRunnerTest.groovy:50)

jbachorik · 2021-03-03T15:07:54Z

👋 All tests are finally passing 🎉

tylerbenson · 2021-03-03T15:11:15Z

+}
+
+apply from: "$rootDir/gradle/java.gradle"
+apply plugin: "idea"


This is probably not needed.

tylerbenson · 2021-03-03T15:12:45Z

-class StreamingSamplerTest {
-
+class AdaptiveSamplerTest {
+  private static final Logger log = LoggerFactory.getLogger(AdaptiveSamplerTest.class);


Use @Slf4j on class instead.

Move the adaptive sampler to a shared location.

a8e7863

The rate limiting sampler is not specific to exception profiling only so it should live in a more generic space.

jbachorik requested review from bantonsson, jpbempel and randomanderson February 18, 2021 15:47

jbachorik requested a review from a team as a code owner February 18, 2021 15:47

jbachorik added 2 commits February 18, 2021 17:12

Move and fix the adaptive sampler test

5867178

Try fixing the test with more gradle magic

368889f

richardstartin reviewed Feb 18, 2021

View reviewed changes

richardstartin previously requested changes Feb 18, 2021

View reviewed changes

richardstartin reviewed Feb 18, 2021

View reviewed changes

Move the adaptive sampler to Java 8 specific module

1feb79c

richardstartin self-requested a review February 19, 2021 11:24

Include adaptive sampler in bootstrap

052472b

Fix the messed up dependency causing tests to fail

031f71e

jbachorik added 2 commits February 19, 2021 18:54

More fiddling around with dependencies

5ef4fc3

Do not pull in transitive java 8 dependency automatically

ffba3a2

richardstartin reviewed Mar 2, 2021

View reviewed changes

jbachorik added 2 commits March 2, 2021 16:32

Quick'nDirty hack to make the agent tests pass on Java 7

65cc424

Adjust the jacoco passing threshold for the sampler

c49a357

tylerbenson approved these changes Mar 3, 2021

View reviewed changes

richardstartin approved these changes Mar 3, 2021

View reviewed changes

jbachorik added 2 commits March 5, 2021 13:43

Addressing minor review comments

ccc4ee6

Spotless!

644786b

jbachorik merged commit 0d6a139 into master Mar 5, 2021

jbachorik deleted the jb/shared_adaptive_sampler branch March 5, 2021 14:05

github-actions Bot added this to the 0.76.0 milestone Mar 5, 2021

tylerbenson added the tag: no release notes Changes to exclude from release notes label Mar 23, 2021

Conversation

jbachorik commented Feb 18, 2021

Uh oh!

richardstartin Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

richardstartin Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

richardstartin left a comment • 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!

richardstartin Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

jbachorik commented Feb 19, 2021

Uh oh!

jbachorik commented Feb 19, 2021

Uh oh!

richardstartin commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbachorik commented Feb 19, 2021

Uh oh!

richardstartin commented Feb 19, 2021

Uh oh!

richardstartin Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

richardstartin commented Mar 2, 2021

Uh oh!

jbachorik commented Mar 3, 2021

Uh oh!

tylerbenson Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

tylerbenson Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richardstartin left a comment •

edited

Loading

richardstartin commented Feb 19, 2021 •

edited

Loading