Make sure the error context is always set

apm-agent-core/src/main/java/co/elastic/apm/impl/ElasticApmTracer.java

@@ -231,8 +231,11 @@ public void captureException(long epochTimestampMillis, @Nullable Exception e) {
             stacktraceFactory.fillStackTrace(error.getException().getStacktrace(), e.getStackTrace());
             Transaction transaction = currentTransaction();
             if (transaction != null) {
+                // The error might have occurred in a different thread than the one the transaction was recorded
+                // That's why we have to ensure the visibility of the transaction properties
+                error.getContext().copyFrom(transaction.getContextEnsureVisibility());
                 error.asChildOf(transaction);
-                error.getContext().copyFrom(transaction.getContext());
+                error.getTransaction().getTransactionId().copyFrom(transaction.getId());
             }
             reporter.report(error);
         }

apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Transaction.java

@@ -109,6 +109,18 @@ public Context getContext() {
         return context;
     }
 
+    /**
+     * Returns the context and ensures visibility when accessed from a different thread.
+     *
+     * @return the transaction context
+     * @see #getContext()
+     */
+    public Context getContextEnsureVisibility() {
+        synchronized (this) {
+            return context;
+        }
+    }
+
     /**
      * UUID for the transaction, referred by its spans
      * (Required)

apm-agent-core/src/main/java/co/elastic/apm/report/serialize/DslJsonSerializer.java

@@ -146,7 +146,7 @@ private void serializeError(ErrorCapture errorCapture) {
     }
 
     private void serializeTransactionReference(ErrorCapture errorCapture) {
-        if (!errorCapture.getTransaction().hasContent()) {
+        if (errorCapture.getTransaction().hasContent()) {
             writeFieldName("transaction");
             jw.writeByte(JsonWriter.OBJECT_START);
             TransactionId transactionId = errorCapture.getTransaction().getTransactionId();

apm-agent-core/src/test/java/co/elastic/apm/AbstractInstrumentationTest.java

@@ -27,16 +27,19 @@
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
+import org.stagemonitor.configuration.ConfigurationRegistry;
 
 public abstract class AbstractInstrumentationTest {
     protected static ElasticApmTracer tracer;
     protected static MockReporter reporter;
+    protected static ConfigurationRegistry config;
 
     @BeforeAll
     static void beforeAll() {
         reporter = new MockReporter();
+        config = SpyConfiguration.createSpyConfig();
         tracer = new ElasticApmTracerBuilder()
-            .configurationRegistry(SpyConfiguration.createSpyConfig())
+            .configurationRegistry(config)
             .reporter(reporter)
             .build();
         ElasticApmAgent.initInstrumentation(tracer, ByteBuddyAgent.install());
@@ -49,6 +52,7 @@ static void afterAll() {
 
     @BeforeEach
     final void resetReporter() {
+        SpyConfiguration.reset(config);
         reporter.reset();
     }
 }

apm-agent-core/src/test/java/co/elastic/apm/configuration/SpyConfiguration.java

@@ -7,9 +7,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -19,6 +19,7 @@
  */
 package co.elastic.apm.configuration;
 
+import org.mockito.Mockito;
 import org.stagemonitor.configuration.ConfigurationOptionProvider;
 import org.stagemonitor.configuration.ConfigurationRegistry;
 import org.stagemonitor.configuration.source.SimpleSource;
@@ -48,4 +49,10 @@ public static ConfigurationRegistry createSpyConfig() {
             .addConfigSource(new SimpleSource(CONFIG_SOURCE_NAME).add("service_name", "elastic-apm-test"))
             .build();
     }
+
+    public static void reset(ConfigurationRegistry config) {
+        for (ConfigurationOptionProvider provider : config.getConfigurationOptionProviders()) {
+            Mockito.reset(provider);
+        }
+    }
 }

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

@@ -7,9 +7,9 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  *      http://www.apache.org/licenses/LICENSE-2.0
- * 
+ *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -54,18 +54,37 @@ static void init(ElasticApmTracer tracer) {
 
     @Nullable
     @Advice.OnMethodEnter
-    public static Transaction onEnterServletService(@Advice.Argument(0) ServletRequest request) {
+    public static Transaction onEnterServletService(@Advice.Argument(0) ServletRequest servletRequest) {
         if (servletTransactionHelper != null &&
-            request instanceof HttpServletRequest &&
-            !Boolean.TRUE.equals(request.getAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST))) {
-            final HttpServletRequest httpServletRequest = (HttpServletRequest) request;
-            final Transaction transaction = servletTransactionHelper.onBefore(httpServletRequest.getServletPath(), httpServletRequest.getPathInfo(),
-                httpServletRequest.getRequestURI(), httpServletRequest.getHeader("User-Agent"),
-                httpServletRequest.getHeader(TraceContext.TRACE_PARENT_HEADER));
+            servletRequest instanceof HttpServletRequest &&
+            !Boolean.TRUE.equals(servletRequest.getAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST))) {
+
+            final HttpServletRequest request = (HttpServletRequest) servletRequest;
+            final Transaction transaction = servletTransactionHelper.onBefore(
+                request.getServletPath(), request.getPathInfo(),
+                request.getRequestURI(), request.getHeader("User-Agent"),
+                request.getHeader(TraceContext.TRACE_PARENT_HEADER));
             if (transaction == null) {
                 // if the request is excluded, avoid matching all exclude patterns again on each filter invocation
-                httpServletRequest.setAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST, Boolean.TRUE);
+                request.setAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST, Boolean.TRUE);
+                return null;
+            }
+            final Request req = transaction.getContext().getRequest();
+            if (transaction.isSampled() && request.getCookies() != null) {
+                for (Cookie cookie : request.getCookies()) {
+                    req.addCookie(cookie.getName(), cookie.getValue());
+                }
+            }
+            final Enumeration headerNames = request.getHeaderNames();
+            while (headerNames.hasMoreElements()) {
+                final String headerName = (String) headerNames.nextElement();
+                req.addHeader(headerName, request.getHeader(headerName));
             }
+
+            final Principal userPrincipal = request.getUserPrincipal();
+            servletTransactionHelper.fillRequestContext(transaction, userPrincipal != null ? userPrincipal.getName() : null,
+                request.getProtocol(), request.getMethod(), request.isSecure(), request.getScheme(), request.getServerName(),
+                request.getServerPort(), request.getRequestURI(), request.getQueryString(), request.getRemoteAddr(), request.getRequestURL());
             return transaction;
         }
         return null;
@@ -76,30 +95,20 @@ public static void onExitServletService(@Advice.Argument(0) ServletRequest servl
                                             @Advice.Argument(1) ServletResponse servletResponse,
                                             @Advice.Enter @Nullable Transaction transaction,
                                             @Advice.Thrown @Nullable Exception exception) {
-        if (servletTransactionHelper != null && transaction != null && servletRequest instanceof HttpServletRequest &&
+        if (servletTransactionHelper != null &&
+            transaction != null &&
+            servletRequest instanceof HttpServletRequest &&
             servletResponse instanceof HttpServletResponse) {
-            final HttpServletRequest request = (HttpServletRequest) servletRequest;
+
             final HttpServletResponse response = (HttpServletResponse) servletResponse;
-            final Request req = transaction.getContext().getRequest();
-            if (request.getCookies() != null) {
-                for (Cookie cookie : request.getCookies()) {
-                    req.addCookie(cookie.getName(), cookie.getValue());
-                }
-            }
-            final Enumeration headerNames = request.getHeaderNames();
-            while (headerNames.hasMoreElements()) {
-                final String headerName = (String) headerNames.nextElement();
-                req.addHeader(headerName, request.getHeader(headerName));
-            }
+            final HttpServletRequest request = (HttpServletRequest) servletRequest;
             final Response resp = transaction.getContext().getResponse();
             for (String headerName : response.getHeaderNames()) {
                 resp.addHeader(headerName, response.getHeader(headerName));
             }
-            final Principal userPrincipal = request.getUserPrincipal();
-            servletTransactionHelper.onAfter(transaction, exception, userPrincipal != null ? userPrincipal.getName() : null,
-                request.getProtocol(), request.getMethod(), request.isSecure(), request.getScheme(), request.getServerName(),
-                request.getServerPort(), request.getRequestURI(), request.getQueryString(), request.getParameterMap(),
-                request.getRemoteAddr(), request.getRequestURL(), response.isCommitted(), response.getStatus());
+
+            servletTransactionHelper.onAfter(transaction, exception, response.isCommitted(), response.getStatus(), request.getMethod(),
+                request.getParameterMap());
         }
     }
 }

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

@@ -63,6 +63,29 @@ public class ServletTransactionHelper {
         this.webConfiguration = tracer.getConfig(WebConfiguration.class);
     }
 
+    /*
+     * As much of the request information as possible should be set before the request processing starts.
+     *
+     * That way, when recording an error,
+     * we can copy the transaction context to the error context.
+     *
+     * This has the advantage that we don't have to create the context for the error again.
+     * As creating the context is framework specific,
+     * this also means less effort when adding support for new frameworks,
+     * because the creating the context is handled in one central place.
+     *
+     * Furthermore, it is not trivial to create an error context at an arbitrary location
+     * (when the user calls ElasticApm.captureException()),
+     * as we don't necessarily have access to the framework's request and response objects.
+     *
+     * Additionally, we only have access to the classes of the instrumented classes inside advice methods.
+     *
+     * Currently, there is no configuration option to disable tracing but to still enable error tracking.
+     * But even when introducing that, the approach of copying the transaction context can still work.
+     * We will then capture the transaction but not report it.
+     * As the capturing of the transaction is garbage free, this should not add a significant overhead.
+     * Also, this setting would be rather niche, as we are a APM solution after all.
+     */
     @Nullable
     @VisibleForAdvice
     public Transaction onBefore(String servletPath, String pathInfo, String requestURI,
@@ -78,34 +101,31 @@ public Transaction onBefore(String servletPath, String pathInfo, String requestU
         }
     }
 
-    /*
-     * filling the transaction after the request has been processed is safer
-     * as reading the parameters could potentially decode them in the wrong encoding
-     * or trigger exceptions,
-     * for example when the amount of query parameters is longer than the application server allows
-     * in that case, we rather want that the agent looks like the cause for this
-     */
     @VisibleForAdvice
-    public void onAfter(Transaction transaction, @Nullable Exception exception,
-                        @Nullable String userName, String protocol, String method, boolean secure, String scheme, String serverName,
-                        int serverPort, String requestURI, String queryString, Map<String, String[]> parameterMap, String remoteAddr,
-                        StringBuffer requestURL, boolean committed, int status) {
-        try {
-            Context context = transaction.getContext();
-            final Request request = transaction.getContext().getRequest();
-            fillRequest(request, protocol, method, secure, scheme, serverName, serverPort, requestURI, queryString, parameterMap,
-                remoteAddr, requestURL);
-
-            fillResponse(context.getResponse(), committed, status);
-            // only set username if not manually set
-            if (context.getUser().getUsername() == null) {
-                context.getUser().withUsername(userName);
-            }
+    public void fillRequestContext(Transaction transaction, @Nullable String userName, String protocol, String method, boolean secure,
+                                   String scheme, String serverName, int serverPort, String requestURI, String queryString,
+                                   String remoteAddr, StringBuffer requestURL) {
+        // the HTTP method is not a good transaction name, but better than none...
+        if (transaction.getName().length() == 0) {
+            transaction.withName(method);
+        }
+        Context context = transaction.getContext();
+        final Request request = transaction.getContext().getRequest();
+        fillRequest(request, protocol, method, secure, scheme, serverName, serverPort, requestURI, queryString, remoteAddr, requestURL);
 
-            // the HTTP method is not a good transaction name, but better than none...
-            if (transaction.getName().length() == 0) {
-                transaction.withName(method);
-            }
+        // only set username if not manually set
+        if (context.getUser().getUsername() == null) {
+            context.getUser().withUsername(userName);
+        }
+    }
+
+
+    @VisibleForAdvice
+    public void onAfter(Transaction transaction, @Nullable Exception exception, boolean committed, int status, String method,
+                        Map<String, String[]> parameterMap) {
+        try {
+            fillRequestParameters(transaction, method, parameterMap);
+            fillResponse(transaction.getContext().getResponse(), committed, status);
             transaction.withResult(ResultUtil.getResultByHttpStatus(status));
             transaction.withType("request");
             if (exception != null) {
@@ -118,6 +138,24 @@ public void onAfter(Transaction transaction, @Nullable Exception exception,
         transaction.end();
     }
 
+    /*
+     * Filling the parameter after the request has been processed is safer
+     * as reading the parameters could potentially decode them in the wrong encoding
+     * or trigger exceptions,
+     * for example when the amount of query parameters is longer than the application server allows.
+     * In that case, we rather not want that the agent looks like the cause for this.
+     */
+    private void fillRequestParameters(Transaction transaction, String method, Map<String, String[]> parameterMap) {
+        Request request = transaction.getContext().getRequest();
+        if (hasBody(request.getHeaders(), method)) {
+            if (webConfiguration.getCaptureBody() != OFF) {
+                captureBody(request, parameterMap);
+            } else {
+                request.redactBody();
+            }
+        }
+    }
+
     private boolean isExcluded(String servletPath, String pathInfo, String requestURI, @Nullable String userAgentHeader) {
         boolean excludeUrl = WildcardMatcher.anyMatch(webConfiguration.getIgnoreUrls(), servletPath, pathInfo);
         if (excludeUrl) {
@@ -140,16 +178,8 @@ private void fillResponse(Response response, boolean committed, int status) {
 
     private void fillRequest(Request request, String protocol, String method, boolean secure,
                              String scheme, String serverName, int serverPort, String requestURI, String queryString,
-                             Map<String, String[]> parameterMap, String remoteAddr, StringBuffer requestURL) {
-        final WebConfiguration.EventType eventType = webConfiguration.getCaptureBody();
+                             String remoteAddr, StringBuffer requestURL) {
 
-        if (hasBody(request.getHeaders(), method)) {
-            if (eventType != OFF) {
-                captureBody(request, parameterMap);
-            } else {
-                request.redactBody();
-            }
-        }
         request.withHttpVersion(getHttpVersion(protocol));
         request.withMethod(method);