Avoid setting request attributes to reduce allocations (#316)

Adding a request attribute creates a new HashMap$Node which allocates 32b
- Only set transaction request attribute when startAsync is called
- Set excluded as ThreadLocal as opposed to request attribute

Author: felixbarny

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/AsyncInstrumentation.java

@@ -23,6 +23,7 @@
 import co.elastic.apm.bci.HelperClassManager;
 import co.elastic.apm.bci.VisibleForAdvice;
 import co.elastic.apm.impl.ElasticApmTracer;
+import co.elastic.apm.impl.transaction.Transaction;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.NamedElement;
 import net.bytebuddy.description.method.MethodDescription;
@@ -37,6 +38,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 
+import static co.elastic.apm.servlet.ServletApiAdvice.TRANSACTION_ATTRIBUTE;
 import static net.bytebuddy.matcher.ElementMatchers.hasSuperType;
 import static net.bytebuddy.matcher.ElementMatchers.isInterface;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
@@ -54,7 +56,7 @@
  * See https://github.com/raphw/byte-buddy/issues/465 for more information.
  * However, the helper class {@link StartAsyncAdviceHelper} has access to the Servlet API,
  * as it is loaded by the child classloader of {@link AsyncContext}
- * (see {@link StartAsyncAdvice#onExitStartAsync(AsyncContext)}).
+ * (see {@link StartAsyncAdvice#onExitStartAsync(HttpServletRequest, AsyncContext)}).
  */
 public class AsyncInstrumentation extends ElasticApmInstrumentation {
 
@@ -122,9 +124,15 @@ public interface StartAsyncAdviceHelper<T> {
     public static class StartAsyncAdvice {
 
         @Advice.OnMethodExit
-        private static void onExitStartAsync(@Advice.Return AsyncContext asyncContext) {
-            if (asyncHelper != null) {
-                asyncHelper.getForClassLoaderOfClass(AsyncContext.class).onExitStartAsync(asyncContext);
+        private static void onExitStartAsync(@Advice.This HttpServletRequest request, @Advice.Return AsyncContext asyncContext) {
+            if (tracer != null) {
+                final Transaction transaction = tracer.currentTransaction();
+                if (transaction != null) {
+                    request.setAttribute(TRANSACTION_ATTRIBUTE, transaction);
+                    if (asyncHelper != null) {
+                        asyncHelper.getForClassLoaderOfClass(AsyncContext.class).onExitStartAsync(asyncContext);
+                    }
+                }
             }
         }
     }

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/FilterChainInstrumentation.java

@@ -20,7 +20,6 @@
 package co.elastic.apm.servlet;
 
 import co.elastic.apm.bci.ElasticApmInstrumentation;
-import co.elastic.apm.bci.VisibleForAdvice;
 import co.elastic.apm.impl.ElasticApmTracer;
 import net.bytebuddy.description.NamedElement;
 import net.bytebuddy.description.method.MethodDescription;
@@ -43,9 +42,6 @@
  */
 public class FilterChainInstrumentation extends ElasticApmInstrumentation {
 
-    @VisibleForAdvice
-    public static final String EXCLUDE_REQUEST = "elastic.apm.servlet.request.exclude";
-
     @Override
     public void init(ElasticApmTracer tracer) {
         ServletApiAdvice.init(tracer);

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletApiAdvice.java

@@ -55,6 +55,13 @@ public class ServletApiAdvice {
     @Nullable
     @VisibleForAdvice
     public static ElasticApmTracer tracer;
+    @VisibleForAdvice
+    public static ThreadLocal<Boolean> excluded = new ThreadLocal<Boolean>() {
+        @Override
+        protected Boolean initialValue() {
+            return Boolean.FALSE;
+        }
+    };
 
     static void init(ElasticApmTracer tracer) {
         ServletApiAdvice.tracer = tracer;
@@ -77,7 +84,7 @@ public static void onEnterServletService(@Advice.Argument(0) ServletRequest serv
         if (servletTransactionHelper != null &&
             servletRequest instanceof HttpServletRequest &&
             servletRequest.getDispatcherType() == DispatcherType.REQUEST &&
-            !Boolean.TRUE.equals(servletRequest.getAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST))) {
+            !Boolean.TRUE.equals(excluded.get())) {
 
             final HttpServletRequest request = (HttpServletRequest) servletRequest;
             transaction = servletTransactionHelper.onBefore(
@@ -86,10 +93,8 @@ public static void onEnterServletService(@Advice.Argument(0) ServletRequest serv
                 request.getHeader(TraceContext.TRACE_PARENT_HEADER));
             if (transaction == null) {
                 // if the request is excluded, avoid matching all exclude patterns again on each filter invocation
-                request.setAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST, Boolean.TRUE);
+                excluded.set(Boolean.TRUE);
                 return;
-            } else {
-                request.setAttribute(TRANSACTION_ATTRIBUTE, transaction);
             }
             final Request req = transaction.getContext().getRequest();
             if (transaction.isSampled() && request.getCookies() != null) {
@@ -121,6 +126,7 @@ public static void onExitServletService(@Advice.Argument(0) ServletRequest servl
         if (tracer == null) {
             return;
         }
+        excluded.set(Boolean.FALSE);
         if (scope != null) {
             scope.close();
         }

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletTransactionHelper.java

@@ -39,6 +39,7 @@
 import javax.annotation.Nullable;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -185,15 +186,17 @@ private void fillRequestParameters(Transaction transaction, String method, Map<S
     }
 
     private boolean isExcluded(String servletPath, String pathInfo, String requestURI, @Nullable String userAgentHeader) {
-        boolean excludeUrl = WildcardMatcher.anyMatch(webConfiguration.getIgnoreUrls(), servletPath, pathInfo) != null;
+        final List<WildcardMatcher> ignoreUrls = webConfiguration.getIgnoreUrls();
+        boolean excludeUrl = WildcardMatcher.anyMatch(ignoreUrls, servletPath, pathInfo) != null;
         if (excludeUrl) {
             logger.debug("Not tracing this request as the URL {} is ignored by one of the matchers",
-                requestURI, webConfiguration.getIgnoreUrls());
+                requestURI, ignoreUrls);
         }
-        boolean excludeAgent = userAgentHeader != null && WildcardMatcher.anyMatch(webConfiguration.getIgnoreUserAgents(), userAgentHeader) != null;
+        final List<WildcardMatcher> ignoreUserAgents = webConfiguration.getIgnoreUserAgents();
+        boolean excludeAgent = userAgentHeader != null && WildcardMatcher.anyMatch(ignoreUserAgents, userAgentHeader) != null;
         if (excludeAgent) {
             logger.debug("Not tracing this request as the User-Agent {} is ignored by one of the matchers",
-                userAgentHeader, webConfiguration.getIgnoreUserAgents());
+                userAgentHeader, ignoreUserAgents);
         }
         return excludeUrl || excludeAgent;
     }

apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java

@@ -49,6 +49,8 @@
 
 import static org.assertj.core.api.Java6Assertions.assertThat;
 import static org.assertj.core.api.Java6Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 class ApmFilterTest extends AbstractInstrumentationTest {
@@ -145,11 +147,14 @@ void testIgnoreUrlStartWithNoMatch() throws IOException, ServletException {
 
     @Test
     void testIgnoreUrlEndWith() throws IOException, ServletException {
+        filterChain = new MockFilterChain(new HttpServlet() {
+        });
         when(webConfiguration.getIgnoreUrls()).thenReturn(Collections.singletonList(WildcardMatcher.valueOf("*.js")));
         final MockHttpServletRequest request = new MockHttpServletRequest();
         request.setServletPath("/resources");
         request.setPathInfo("test.js");
         filterChain.doFilter(request, new MockHttpServletResponse());
+        verify(webConfiguration, times(1)).getIgnoreUrls();
         assertThat(reporter.getTransactions()).hasSize(0);
     }