Merge pull request #2025 from newrelic/slick-3.5.0-bug

Slick 3.5.0 bug fix

Author: kanderson250

instrumentation/slick-2.12_3.2.0/build.gradle

@@ -25,10 +25,10 @@ verifyInstrumentation {
     fails 'com.typesafe.slick:slick_2.11:[3.2.0,)'
 
     // scala 12
-    passesOnly 'com.typesafe.slick:slick_2.12:[3.2.0,)'
+    passesOnly 'com.typesafe.slick:slick_2.12:[3.2.0,3.5.0)'
 
     // scala 13
-    passesOnly 'com.typesafe.slick:slick_2.13:[3.3.2,)'
+    passesOnly 'com.typesafe.slick:slick_2.13:[3.3.2,3.5.0)'
 
     excludeRegex ".*(RC|M)[0-9].*"
 }

instrumentation/slick-2.12_3.2.0/src/main/scala/slick/util/SkipDefaultAsyncExecutor.java

@@ -0,0 +1,15 @@
+/*
+ *
+ *  * Copyright 2024 New Relic Corporation. All rights reserved.
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+
+
+package slick.util;
+
+import com.newrelic.api.agent.weaver.SkipIfPresent;
+
+@SkipIfPresent(originalName = "slick.util.AsyncExecutor$DefaultAsyncExecutor")
+public class SkipDefaultAsyncExecutor {
+}

instrumentation/slick-2.12_3.5.0/README.md

@@ -0,0 +1,11 @@
+# Slick 3.5.0 Instrumentation
+
+This instrumentation hooks into the `run` Function parameter of `AsyncExecutor.prioritizedRunnable`. 
+We create a segment, named `ORM/Slick/SlickQuery`, whenever the `run` Function is eventually called. 
+
+Previous versions of the Slick instrumentation wrapped various executors, execution contexts, and runnables
+related to Slick's `AsyncExecutor`. This created casting issues in Slick 3.5.0+, because our wrappers effectively upcasted
+these types, which have concrete implementations in the Slick source code. Our wrappers ignored critical overridden methods, 
+falling through to the default implementations and disrupting Slick's underlying concurrency mechanism. 
+
+To avoid these casting issues, we are no longer wrapping anything, except the `run` Function.

instrumentation/slick-2.12_3.5.0/build.gradle

@@ -0,0 +1,39 @@
+apply plugin: 'scala'
+scala.zincVersion = "1.7.1"
+
+isScalaProjectEnabled(project, "scala-2.13")
+
+dependencies {
+    implementation(project(":newrelic-api"))
+    implementation(project(":agent-bridge"))
+    implementation(project(":newrelic-weaver-api"))
+    implementation(project(":newrelic-weaver-scala-api"))
+    implementation("org.scala-lang:scala-library:2.13.0")
+    implementation("com.typesafe.slick:slick_2.13:3.5.1")
+
+    testImplementation("com.h2database:h2:1.4.190")
+    testImplementation(project(":instrumentation:jdbc-h2")) { transitive = false }
+    testImplementation(project(":instrumentation:jdbc-generic")) { transitive = false }
+}
+
+jar {
+    manifest { attributes 'Implementation-Title': 'com.newrelic.instrumentation.slick-2.12_3.5.0' }
+}
+
+verifyInstrumentation {
+    // scala 11 should be instrumented by another module
+    fails 'com.typesafe.slick:slick_2.11:[3.2.0,)'
+
+    // scala 12
+    passesOnly 'com.typesafe.slick:slick_2.12:[3.5.0,)'
+
+    // scala 13
+    passesOnly 'com.typesafe.slick:slick_2.13:[3.5.0,)'
+
+    excludeRegex ".*(RC|M)[0-9].*"
+}
+
+site {
+    title 'Slick'
+    type 'Datastore'
+}

instrumentation/slick-2.12_3.5.0/src/main/scala/slick/util/AsyncExecutorUtil.scala

@@ -0,0 +1,29 @@
+/*
+ *
+ *  * Copyright 2024 New Relic Corporation. All rights reserved.
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+
+
+package slick.util
+
+import com.newrelic.api.agent.{NewRelic, Token, Trace}
+import slick.util.AsyncExecutor.PrioritizedRunnable
+import slick.util.AsyncExecutor.PrioritizedRunnable.SetConnectionReleased
+
+object AsyncExecutorUtil{
+
+  def wrapRunMethod(run: SetConnectionReleased => Unit, token: Token): SetConnectionReleased => Unit = setConnectionReleased => {
+    doRun(run, setConnectionReleased, token)
+  }
+
+  @Trace(async = true)
+  def doRun(run: SetConnectionReleased => Unit, setConnectionReleased: SetConnectionReleased, token: Token): Unit = {
+    if (token != null) {
+      token.linkAndExpire();
+    }
+    NewRelic.getAgent.getTracedMethod.setMetricName("ORM", "Slick", "slickQuery")
+    run(setConnectionReleased)
+  }
+}

instrumentation/slick-2.12_3.5.0/src/main/scala/slick/util/AsyncExecutor_Instrumentation.java

@@ -0,0 +1,33 @@
+/*
+ *
+ *  * Copyright 2024 New Relic Corporation. All rights reserved.
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+
+
+package slick.util;
+
+import com.newrelic.agent.bridge.AgentBridge;
+import com.newrelic.api.agent.Token;
+import com.newrelic.api.agent.Transaction;
+import com.newrelic.api.agent.weaver.MatchType;
+import com.newrelic.api.agent.weaver.Weave;
+import com.newrelic.api.agent.weaver.Weaver;
+
+import scala.Function0;
+import scala.Function1;
+import slick.util.AsyncExecutor.PrioritizedRunnable;
+
+@Weave(type = MatchType.Interface, originalName = "slick.util.AsyncExecutor")
+public class AsyncExecutor_Instrumentation {
+    public PrioritizedRunnable prioritizedRunnable(Function0 priority, Function1 run) {
+        Transaction txn = AgentBridge.getAgent().getTransaction(false);
+        Token token = null;
+        if ( txn != null) {
+            token = txn.getToken();
+            run = AsyncExecutorUtil.wrapRunMethod(run, token);
+        }
+        return Weaver.callOriginal();
+    }
+}

instrumentation/slick-2.12_3.5.0/src/test/scala/com/nr/agent/instrumentation/slickdb/SlickTest_350.scala

@@ -0,0 +1,161 @@
+/*
+ *
+ *  * Copyright 2024 New Relic Corporation. All rights reserved.
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+
+package com.nr.agent.instrumentation.slickdb
+
+import com.newrelic.agent.introspec.InstrumentationTestConfig
+import com.newrelic.agent.introspec.InstrumentationTestRunner
+import com.newrelic.agent.introspec.Introspector;
+import com.newrelic.api.agent.Trace;
+
+import scala.language.postfixOps
+
+import org.junit._
+import org.junit.runner.RunWith;
+
+import scala.concurrent._
+import scala.concurrent.duration._
+import scala.concurrent.ExecutionContext.Implicits.global;
+import slick.jdbc.H2Profile.api._
+
+// Copied from slick-3.0.0 module
+@RunWith(classOf[InstrumentationTestRunner])
+@InstrumentationTestConfig(includePrefixes = Array("slick", "org.h2"))
+class SlickTest_350 {
+  import SlickTest_350.slickdb
+  import SlickTest_350.users
+
+  @Test
+  def testCrud() {
+    slickInsert();
+    slickUpdate();
+    slickDelete();
+    Await.result(slickResult(), 20 seconds)
+    val introspector :Introspector = InstrumentationTestRunner.getIntrospector()
+    awaitFinishedTx(introspector, 4);
+    val txnNames = introspector.getTransactionNames()
+    txnNames.forEach(name => {
+      val metrics = introspector.getMetricsForTransaction(name)
+      Assert.assertTrue(metrics.containsKey("ORM/Slick/slickQuery"))
+    })
+  }
+
+  /*
+  This test runs 50 concurrent queries, exceeding the default number of db connections provided by slick (20).
+  A bug was discovered in the previous instrumentation for slick versions 3.5.0+, where for slick queries occurring
+  outside a transaction, the executor would eventually pause and stop taking up any new work (despite having available
+  threads). This module was created in response to that bug (see the README) and this test captures the bug's behavior.
+  It will fail if run with previous instrumentation and slick <3.5.0.
+   */
+  @Test
+  def testNoTxn(): Unit = {
+    try {
+      Await.result(runConcurrentQueries, 10.seconds)
+    } catch {
+      case _: Throwable => Assert.fail("Futures timed out running concurrent queries.")
+    }
+
+  }
+
+
+  @Trace(dispatcher = true)
+  def slickResult() :Future[String] = {
+    slickdb.run(users.result).map(units => {
+      var res :String = ""
+      units.foreach {
+        case (id, first_name, last_name) =>
+          res += " * " + id + ": " + first_name + " " + last_name + "\n"
+      }
+      "Got results: \n"+res
+    })
+  }
+
+  @Trace(dispatcher = true)
+  def slickInsert() :Future[String] = {
+    slickdb.run(users.map(u => (u.id, u.first_name, u.last_name)) += (4, "John", "JacobJingle")).map(rowsInserted => {
+      "Table now has "+rowsInserted+" users"
+    })
+  }
+
+  @Trace(dispatcher = true)
+  def slickUpdate() :Future[String] = {
+    slickdb.run(users.filter(_.id === 1).map(u => (u.first_name)).update(("Fred"))).map(result => {
+      "result: "+result
+    })
+  }
+
+  @Trace(dispatcher = true)
+  def slickDelete() :Future[String] = {
+    // people.filter(p => p.name === "M. Odersky").delete
+    slickdb.run(users.filter(_.id === 2).delete).map(result => {
+      "result: "+result
+    })
+  }
+
+  def testQuery(id: Int) = {
+    users.filter(_.id === id)
+  }
+
+  def runConcurrentQueries = Future.traverse(1 to 50) { x =>
+    val whichId = (x % 3) + 1
+    slickdb.run(testQuery(whichId).result).map { v => println(s"Query Result $x: " + v) }
+  }
+
+  // introspector does not handle async tx finishing very well so we're sleeping as a workaround
+  private def awaitFinishedTx(introspector :Introspector, expectedTxCount: Int = 1) {
+    while(introspector.getFinishedTransactionCount() <= expectedTxCount-1) {
+      Thread.sleep(100)
+    }
+    Thread.sleep(100)
+  }
+
+}
+
+class Users(tag: Tag) extends Table[(Int, String, String)] (tag, "user") {
+  def id = column[Int]("id", O.PrimaryKey)
+  def first_name = column[String]("first_name")
+  def last_name = column[String]("last_name")
+  // Every table needs a * projection with the same type as the table's type parameter
+  def * = (id, first_name, last_name)
+}
+
+object SlickTest_350 {
+  val DB_DRIVER: String = "org.h2.Driver";
+  val DB_CONNECTION: String = "jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;DATABASE_TO_UPPER=false";
+
+  val slickdb = Database.forURL(DB_CONNECTION, DB_DRIVER)
+  val users = TableQuery[Users]
+
+  @BeforeClass
+  def setup() {
+    // set up h2
+    Assert.assertNotNull("Unable to create h2 db.", slickdb)
+    Assert.assertNotNull("Unable to create user table.", users)
+    Await.result(initData(), 10.seconds) //make sure we don't enter the test suite until the init task finishes
+  }
+
+  @AfterClass
+  def teardown() {
+    // tear down h2
+    if (null != slickdb) {
+      slickdb.close();
+    }
+  }
+
+  def initData() = {
+    val setup = DBIO.seq(
+      // Create and populate the tables
+      users.schema.create,
+      users ++= Seq(
+        (1, "Fakus", "Namus"),
+        (2, "Some", "Guy"),
+        (3, "Whatser", "Name")
+      ))
+    slickdb.run(setup)
+  }
+
+}

settings.gradle

@@ -339,6 +339,7 @@ include 'instrumentation:servlet-user-5.0'
 include 'instrumentation:slick-3.0.0'
 include 'instrumentation:slick-2.11_3.2.0'
 include 'instrumentation:slick-2.12_3.2.0'
+include 'instrumentation:slick-2.12_3.5.0'
 include 'instrumentation:solr-4.0.0'
 include 'instrumentation:solr-5.0.0'
 include 'instrumentation:solr-5.1.0'