Enhance/document msg listener heuristic (elastic#3299)

jackshirazi · Aug 25, 2023 · b752467 · b752467
1 parent 3a2163e
commit b752467
Show file tree

Hide file tree

Showing 14 changed files with 90 additions and 32 deletions.
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
@@ -52,6 +52,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 [float]
 ===== Features
 * Virtual thread support - {pull}3244[#3244], {pull}3286[#3286]
+* Include `application_packages` in JMS listener naming heuristic - {pull}3299[#3299]
 
 [float]
 ===== Bug fixes

diff --git a/...pm-jms-jakarta/src/test/java/co/elastic/apm/agent/jms/jakarta/JmsMessageListenerTest.java b/...pm-jms-jakarta/src/test/java/co/elastic/apm/agent/jms/jakarta/JmsMessageListenerTest.java
@@ -22,21 +22,21 @@
 import co.elastic.apm.agent.bci.ElasticApmAgent;
 import co.elastic.apm.agent.impl.ElasticApmTracer;
 import co.elastic.apm.agent.impl.Tracer;
+import co.elastic.apm.agent.impl.stacktrace.StacktraceConfiguration;
 import co.elastic.apm.agent.impl.transaction.Transaction;
-import co.elastic.apm.agent.jms.jakarta.test.TestMessageConsumer;
-import co.elastic.apm.agent.jms.jakarta.test.TestMessageListener;
-import co.elastic.apm.agent.jms.jakarta.test.TestMsgHandler;
+import testapp.TestMessageConsumer;
+import testapp.TestMessageListener;
+import testapp.TestMsgHandler;
 import co.elastic.apm.agent.tracer.GlobalTracer;
 import co.elastic.apm.agent.tracer.configuration.MessagingConfiguration;
 import net.bytebuddy.agent.ByteBuddyAgent;
-import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.*;
 import org.stagemonitor.configuration.ConfigurationRegistry;
 
 import jakarta.jms.Message;
 import jakarta.jms.MessageListener;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static co.elastic.apm.agent.testutils.assertions.Assertions.assertThat;
@@ -70,9 +70,11 @@ private void startAgent(){
 
     @Test
     public void testJmsMessageListenerPackage_defaultConfig() throws Exception {
+        // default configuration
+        doReturn(Collections.emptyList()).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
+
         startAgent();
 
-        // default configuration
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.MATCHING_NAME_CONVENTION);
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.INNER_CLASS);
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.LAMBDA);
@@ -81,7 +83,20 @@ public void testJmsMessageListenerPackage_defaultConfig() throws Exception {
 
     @Test
     public void testJmsMessageListenerPackage_customValue() throws Exception {
-        doReturn(Arrays.asList("co.elastic.apm.agent.jms.jakarta.test")).when(config.getConfig(MessagingConfiguration.class)).getJmsListenerPackages();
+        doReturn(Collections.emptyList()).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
+        doReturn(Arrays.asList("testapp")).when(config.getConfig(MessagingConfiguration.class)).getJmsListenerPackages();
+
+        startAgent();
+
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.MATCHING_NAME_CONVENTION);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.INNER_CLASS);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.LAMBDA);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.NOT_MATCHING_NAME_CONVENTION);
+    }
+
+    @Test
+    public void testJmsMessageListenerPackage_applicationPackages() throws Exception {
+        doReturn(Arrays.asList("testapp")).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
 
         startAgent();
 

diff --git a/...jms/jakarta/test/TestMessageConsumer.java → ...est/java/testapp/TestMessageConsumer.java b/...jms/jakarta/test/TestMessageConsumer.java → ...est/java/testapp/TestMessageConsumer.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.jakarta.test;
+package testapp;
 
 import jakarta.jms.JMSException;
 import jakarta.jms.Message;

diff --git a/...jms/jakarta/test/TestMessageListener.java → ...est/java/testapp/TestMessageListener.java b/...jms/jakarta/test/TestMessageListener.java → ...est/java/testapp/TestMessageListener.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.jakarta.test;
+package testapp;
 
 import co.elastic.apm.agent.impl.Tracer;
 import co.elastic.apm.agent.impl.transaction.Transaction;

diff --git a/...gent/jms/jakarta/test/TestMsgHandler.java → ...src/test/java/testapp/TestMsgHandler.java b/...gent/jms/jakarta/test/TestMsgHandler.java → ...src/test/java/testapp/TestMsgHandler.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.jakarta.test;
+package testapp;
 
 
 import co.elastic.apm.agent.impl.Tracer;

diff --git a/...javax/src/main/java/co/elastic/apm/agent/jms/javax/JmsMessageListenerInstrumentation.java b/...javax/src/main/java/co/elastic/apm/agent/jms/javax/JmsMessageListenerInstrumentation.java
@@ -39,9 +39,6 @@
 
 public class JmsMessageListenerInstrumentation extends BaseJmsInstrumentation {
 
-    @SuppressWarnings("WeakerAccess")
-    public static final Logger logger = LoggerFactory.getLogger(JmsMessageListenerInstrumentation.class);
-
     @Override
     public ElementMatcher<? super NamedElement> getTypeMatcherPreFilter() {
         return getMessageListenerTypeMatcherPreFilter();

diff --git a/...in/apm-jms-javax/src/test/java/co/elastic/apm/agent/jms/javax/JmsMessageListenerTest.java b/...in/apm-jms-javax/src/test/java/co/elastic/apm/agent/jms/javax/JmsMessageListenerTest.java
@@ -20,14 +20,15 @@
 
 import co.elastic.apm.agent.MockTracer;
 import co.elastic.apm.agent.bci.ElasticApmAgent;
+import co.elastic.apm.agent.impl.stacktrace.StacktraceConfiguration;
 import co.elastic.apm.agent.tracer.configuration.MessagingConfiguration;
 import co.elastic.apm.agent.impl.ElasticApmTracer;
 import co.elastic.apm.agent.impl.Tracer;
 import co.elastic.apm.agent.tracer.GlobalTracer;
 import co.elastic.apm.agent.impl.transaction.Transaction;
-import co.elastic.apm.agent.jms.javax.test.TestMessageConsumer;
-import co.elastic.apm.agent.jms.javax.test.TestMessageListener;
-import co.elastic.apm.agent.jms.javax.test.TestMsgHandler;
+import testapp.TestMessageConsumer;
+import testapp.TestMessageListener;
+import testapp.TestMsgHandler;
 import net.bytebuddy.agent.ByteBuddyAgent;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -37,6 +38,7 @@
 import javax.jms.Message;
 import javax.jms.MessageListener;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static co.elastic.apm.agent.testutils.assertions.Assertions.assertThat;
@@ -70,9 +72,11 @@ private void startAgent(){
 
     @Test
     public void testJmsMessageListenerPackage_defaultConfig() throws Exception {
+        // default configuration
+        doReturn(Collections.emptyList()).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
+
         startAgent();
 
-        // default configuration
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.MATCHING_NAME_CONVENTION);
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.INNER_CLASS);
         testJmsMessageListenerPackage(true, JmsMessageListenerVariant.LAMBDA);
@@ -81,7 +85,20 @@ public void testJmsMessageListenerPackage_defaultConfig() throws Exception {
 
     @Test
     public void testJmsMessageListenerPackage_customValue() throws Exception {
-        doReturn(Arrays.asList("co.elastic.apm.agent.jms.javax.test")).when(config.getConfig(MessagingConfiguration.class)).getJmsListenerPackages();
+        doReturn(Collections.emptyList()).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
+        doReturn(Arrays.asList("testapp")).when(config.getConfig(MessagingConfiguration.class)).getJmsListenerPackages();
+
+        startAgent();
+
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.MATCHING_NAME_CONVENTION);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.INNER_CLASS);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.LAMBDA);
+        testJmsMessageListenerPackage(true, JmsMessageListenerVariant.NOT_MATCHING_NAME_CONVENTION);
+    }
+
+    @Test
+    public void testJmsMessageListenerPackage_applicationPackages() throws Exception {
+        doReturn(Arrays.asList("testapp")).when(config.getConfig(StacktraceConfiguration.class)).getApplicationPackages();
 
         startAgent();
 

diff --git a/...t/jms/javax/test/TestMessageConsumer.java → ...est/java/testapp/TestMessageConsumer.java b/...t/jms/javax/test/TestMessageConsumer.java → ...est/java/testapp/TestMessageConsumer.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.javax.test;
+package testapp;
 
 import javax.jms.JMSException;
 import javax.jms.Message;

diff --git a/...t/jms/javax/test/TestMessageListener.java → ...est/java/testapp/TestMessageListener.java b/...t/jms/javax/test/TestMessageListener.java → ...est/java/testapp/TestMessageListener.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.javax.test;
+package testapp;
 
 import co.elastic.apm.agent.impl.Tracer;
 import co.elastic.apm.agent.tracer.GlobalTracer;

diff --git a/.../agent/jms/javax/test/TestMsgHandler.java → ...src/test/java/testapp/TestMsgHandler.java b/.../agent/jms/javax/test/TestMsgHandler.java → ...src/test/java/testapp/TestMsgHandler.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package co.elastic.apm.agent.jms.javax.test;
+package testapp;
 
 
 import co.elastic.apm.agent.impl.Tracer;

diff --git a/...in/apm-jms-plugin-base/src/main/java/co/elastic/apm/agent/jms/BaseJmsInstrumentation.java b/...in/apm-jms-plugin-base/src/main/java/co/elastic/apm/agent/jms/BaseJmsInstrumentation.java
@@ -23,6 +23,7 @@
 import co.elastic.apm.agent.tracer.Tracer;
 import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import co.elastic.apm.agent.tracer.configuration.MessagingConfiguration;
+import co.elastic.apm.agent.tracer.configuration.StacktraceConfiguration;
 import net.bytebuddy.description.NamedElement;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -33,20 +34,17 @@
 
 import static co.elastic.apm.agent.sdk.bytebuddy.CustomElementMatchers.classLoaderCanLoadClass;
 import static co.elastic.apm.agent.sdk.bytebuddy.CustomElementMatchers.isInAnyPackage;
-import static net.bytebuddy.matcher.ElementMatchers.isBootstrapClassLoader;
-import static net.bytebuddy.matcher.ElementMatchers.isPublic;
-import static net.bytebuddy.matcher.ElementMatchers.nameContains;
-import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.not;
-import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
+import static net.bytebuddy.matcher.ElementMatchers.*;
 
 public abstract class BaseJmsInstrumentation extends ElasticApmInstrumentation {
 
     protected final MessagingConfiguration messagingConfiguration;
+    private final StacktraceConfiguration stacktraceConfiguration;
 
     protected BaseJmsInstrumentation() {
         Tracer tracer = GlobalTracer.get();
         messagingConfiguration = tracer.getConfig(MessagingConfiguration.class);
+        stacktraceConfiguration = tracer.getConfig(StacktraceConfiguration.class);
     }
 
     @Override
@@ -84,6 +82,11 @@ public ElementMatcher<? super NamedElement> getMessageListenerTypeMatcherPreFilt
                 .or(nameContains("Message"))
                 .or(nameContains("Listener"));
 
+        Collection<String> applicationPackages = stacktraceConfiguration.getApplicationPackages();
+        if (!applicationPackages.isEmpty()) {
+            nameHeuristic = nameHeuristic.or(isInAnyPackage(applicationPackages, ElementMatchers.<NamedElement>none()));
+        }
+
         Collection<String> listenerPackages = messagingConfiguration.getJmsListenerPackages();
         if (listenerPackages.isEmpty()) {
             // default heuristic

diff --git a/...racer/src/main/java/co/elastic/apm/agent/tracer/configuration/MessagingConfiguration.java b/...racer/src/main/java/co/elastic/apm/agent/tracer/configuration/MessagingConfiguration.java
@@ -100,8 +100,11 @@ public class MessagingConfiguration extends ConfigurationOptionProvider {
         .configurationCategory(MESSAGING_CATEGORY)
         .description("Defines which packages contain JMS MessageListener implementations for instrumentation." +
             "\n" +
-            "When set to a non-empty value, only the classes matching configuration will be instrumented.\n" +
-            "This configuration option helps to make MessageListener type matching faster and improve application startup performance."
+            "When empty (default), all inner-classes or any classes that have 'Listener' or 'Message' in their names are considered.\n"+
+            "\n" +
+            "This configuration option helps to make MessageListener type matching faster and improve application startup performance.\n" +
+            "\n" +
+            "Starting from version 1.43.0, the classes that are part of the 'application_packages' option are also included in the list of classes considered."
         )
         .dynamic(false)
         .buildWithDefault(Collections.<String>emptyList());

diff --git a/docs/configuration.asciidoc b/docs/configuration.asciidoc
@@ -2454,9 +2454,12 @@ Prepending an element with `(?-i)` makes the matching case sensitive.
 ==== `jms_listener_packages` (performance added[1.36.0])
 
 Defines which packages contain JMS MessageListener implementations for instrumentation.
-When set to a non-empty value, only the classes matching configuration will be instrumented.
+When empty (default), only the inner-classes or that have 'Listener' or 'Message' in their names are considered.
+
 This configuration option helps to make MessageListener type matching faster and improve application startup performance.
 
+Starting from version 1.43.0, the classes that are part of the 'application_packages' option are also included.
+
 
 
 
@@ -4546,8 +4549,11 @@ Example: `5ms`.
 # ignore_message_queues=
 
 # Defines which packages contain JMS MessageListener implementations for instrumentation.
-# When set to a non-empty value, only the classes matching configuration will be instrumented.
+# When empty (default), only the inner-classes or that have 'Listener' or 'Message' in their names are considered.
+# 
 # This configuration option helps to make MessageListener type matching faster and improve application startup performance.
+# 
+# Starting from version 1.43.0, the classes that are part of the 'application_packages' option are also included.
 #
 # This setting can not be changed at runtime. Changes require a restart of the application.
 # Type: comma separated list

diff --git a/docs/troubleshooting.asciidoc b/docs/troubleshooting.asciidoc
@@ -133,6 +133,22 @@ Here, we will refer to the agent log file as `/tmp/agent.log`, but any other loc
 . execute a few transactions which aren't properly captured by the agent
 . copy the `/tmp/agent.log` file and send it back for investigation
 
+[float]
+[[trouble-shooting-matching-heuristics]]
+=== Agent matching heuristics
+
+The agent relies on heuristics to define which classes needs to be instrumented or not efficiently to prevent instrumentation
+overhead.
+
+Those heuristics are based on the packages and class names and are influenced by `application_packages` and
+`jms_listener_packages` configurations. However, if instrumentation is not applied as expected or if you want to investigate creating configurations
+without proper knowledge of the application internals, it could be relevant to disable those heuristics for investigation:
+
+1. disable name heuristics by setting `enable_type_matching_name_pre_filtering=false` and enable the <<trouble-shooting-logging-procedure, agent logs>>.
+2. restart the application, which will be slower than usual due to the extra overhead
+3. analyze the agent logs to identify which classes/methods are instrumented by filtering lines with `Method match` string.
+4. properly configure `application_packages` or `jms_listener_packages` with values that apply
+
 [float]
 [[trouble-shooting-debugging]]
 === Debugging