diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/ElasticApmTracer.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/ElasticApmTracer.java index cd9581a558..9c99f86c5a 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/impl/ElasticApmTracer.java +++ b/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); } diff --git a/apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Transaction.java b/apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Transaction.java index b2c9181d4f..297d59c1b1 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/impl/transaction/Transaction.java +++ b/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) diff --git a/apm-agent-core/src/main/java/co/elastic/apm/report/serialize/DslJsonSerializer.java b/apm-agent-core/src/main/java/co/elastic/apm/report/serialize/DslJsonSerializer.java index 6b4eb1489a..18c05ecbc9 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/report/serialize/DslJsonSerializer.java +++ b/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(); diff --git a/apm-agent-core/src/test/java/co/elastic/apm/AbstractInstrumentationTest.java b/apm-agent-core/src/test/java/co/elastic/apm/AbstractInstrumentationTest.java index 1599c1ad3b..e4a2915023 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/AbstractInstrumentationTest.java +++ b/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(); } } diff --git a/apm-agent-core/src/test/java/co/elastic/apm/configuration/SpyConfiguration.java b/apm-agent-core/src/test/java/co/elastic/apm/configuration/SpyConfiguration.java index 181fd85010..622515720f 100644 --- a/apm-agent-core/src/test/java/co/elastic/apm/configuration/SpyConfiguration.java +++ b/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); + } + } } diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletApiAdvice.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletApiAdvice.java index 17c658c900..510cb60527 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletApiAdvice.java +++ b/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()); } } } diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletTransactionHelper.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletTransactionHelper.java index a0f363a3e4..ee6fd4fe27 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletTransactionHelper.java +++ b/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 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 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 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 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); diff --git a/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java b/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java index 620801dade..6b2de545db 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java @@ -164,7 +164,6 @@ void testIgnoreUserAgentInfix() throws IOException, ServletException { assertThat(reporter.getTransactions()).hasSize(0); } - @Test void testDoNotOverrideUsername() throws IOException, ServletException { filterChain = new MockFilterChain(new HttpServlet() { @@ -180,4 +179,54 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se assertThat(reporter.getFirstTransaction().getContext().getUser().getEmail()).isEqualTo("email"); assertThat(reporter.getFirstTransaction().getContext().getUser().getUsername()).isEqualTo("username"); } + + @Test + void testExceptionCapturingShouldContainContextInformation() throws IOException, ServletException { + filterChain = new MockFilterChain(new HttpServlet() { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) { + tracer.captureException(new RuntimeException("Test exception capturing")); + } + }); + + filterChain.doFilter(new MockHttpServletRequest("GET", "/foo"), new MockHttpServletResponse()); + assertThat(reporter.getTransactions()).hasSize(1); + assertThat(reporter.getErrors()).hasSize(1); + assertThat(reporter.getFirstError().getContext().getRequest().getUrl().getPathname()).isEqualTo("/foo"); + assertThat(reporter.getFirstError().getTransaction().getTransactionId()).isEqualTo(reporter.getFirstTransaction().getId()); + } + + @Test + void testExceptionCapturingShouldContainUserInformationRecordedOnTheTransaction() throws IOException, ServletException { + filterChain = new MockFilterChain(new HttpServlet() { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) { + tracer.currentTransaction().setUser("id", "email", "username"); + tracer.captureException(new RuntimeException("Test exception capturing")); + } + }); + + filterChain.doFilter(new MockHttpServletRequest("GET", "/foo"), new MockHttpServletResponse()); + assertThat(reporter.getTransactions()).hasSize(1); + assertThat(reporter.getErrors()).hasSize(1); + assertThat(reporter.getFirstError().getContext().getUser().getId()).isEqualTo("id"); + assertThat(reporter.getFirstError().getContext().getUser().getEmail()).isEqualTo("email"); + assertThat(reporter.getFirstError().getContext().getUser().getUsername()).isEqualTo("username"); + } + + @Test + void exceptionCapturingShouldNotContainUserInformationRecordedOnTheTransactionAfterTheErrorWasCaptured() throws IOException, ServletException { + filterChain = new MockFilterChain(new HttpServlet() { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) { + tracer.captureException(new RuntimeException("Test exception capturing")); + tracer.currentTransaction().setUser("id", "email", "username"); + } + }); + + filterChain.doFilter(new MockHttpServletRequest("GET", "/foo"), new MockHttpServletResponse()); + assertThat(reporter.getTransactions()).hasSize(1); + assertThat(reporter.getErrors()).hasSize(1); + assertThat(reporter.getFirstError().getContext().getUser().hasContent()).isFalse(); + } }