Skip to content

Commit a6b3758

Browse files
felixbarnyfelixbarny
authored
Make sure the error context is always set (#114)
Previously, most of the transaction context was set after the execution of a request. When capturing an exception, the transaction context is copied over at its current state. That means when the user captures an exception, the transaction context is not yet populated and therefore, the Error context is empty. This changes the initialization of the transaction context so that it is set before the request starts. That way, we can be sure that the Error context is always set
1 parent 54c1fce commit a6b3758

File tree

8 files changed

+183
-69
lines changed

8 files changed

+183
-69
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,11 @@ public void captureException(long epochTimestampMillis, @Nullable Exception e) {
231231
stacktraceFactory.fillStackTrace(error.getException().getStacktrace(), e.getStackTrace());
232232
Transaction transaction = currentTransaction();
233233
if (transaction != null) {
234+
// The error might have occurred in a different thread than the one the transaction was recorded
235+
// That's why we have to ensure the visibility of the transaction properties
236+
error.getContext().copyFrom(transaction.getContextEnsureVisibility());
234237
error.asChildOf(transaction);
235-
error.getContext().copyFrom(transaction.getContext());
238+
error.getTransaction().getTransactionId().copyFrom(transaction.getId());
236239
}
237240
reporter.report(error);
238241
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,18 @@ public Context getContext() {
109109
return context;
110110
}
111111

112+
/**
113+
* Returns the context and ensures visibility when accessed from a different thread.
114+
*
115+
* @return the transaction context
116+
* @see #getContext()
117+
*/
118+
public Context getContextEnsureVisibility() {
119+
synchronized (this) {
120+
return context;
121+
}
122+
}
123+
112124
/**
113125
* UUID for the transaction, referred by its spans
114126
* (Required)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ private void serializeError(ErrorCapture errorCapture) {
146146
}
147147

148148
private void serializeTransactionReference(ErrorCapture errorCapture) {
149-
if (!errorCapture.getTransaction().hasContent()) {
149+
if (errorCapture.getTransaction().hasContent()) {
150150
writeFieldName("transaction");
151151
jw.writeByte(JsonWriter.OBJECT_START);
152152
TransactionId transactionId = errorCapture.getTransaction().getTransactionId();

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,19 @@
2727
import org.junit.jupiter.api.AfterAll;
2828
import org.junit.jupiter.api.BeforeAll;
2929
import org.junit.jupiter.api.BeforeEach;
30+
import org.stagemonitor.configuration.ConfigurationRegistry;
3031

3132
public abstract class AbstractInstrumentationTest {
3233
protected static ElasticApmTracer tracer;
3334
protected static MockReporter reporter;
35+
protected static ConfigurationRegistry config;
3436

3537
@BeforeAll
3638
static void beforeAll() {
3739
reporter = new MockReporter();
40+
config = SpyConfiguration.createSpyConfig();
3841
tracer = new ElasticApmTracerBuilder()
39-
.configurationRegistry(SpyConfiguration.createSpyConfig())
42+
.configurationRegistry(config)
4043
.reporter(reporter)
4144
.build();
4245
ElasticApmAgent.initInstrumentation(tracer, ByteBuddyAgent.install());
@@ -49,6 +52,7 @@ static void afterAll() {
4952

5053
@BeforeEach
5154
final void resetReporter() {
55+
SpyConfiguration.reset(config);
5256
reporter.reset();
5357
}
5458
}

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
* Licensed under the Apache License, Version 2.0 (the "License");
88
* you may not use this file except in compliance with the License.
99
* You may obtain a copy of the License at
10-
*
10+
*
1111
* http://www.apache.org/licenses/LICENSE-2.0
12-
*
12+
*
1313
* Unless required by applicable law or agreed to in writing, software
1414
* distributed under the License is distributed on an "AS IS" BASIS,
1515
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -19,6 +19,7 @@
1919
*/
2020
package co.elastic.apm.configuration;
2121

22+
import org.mockito.Mockito;
2223
import org.stagemonitor.configuration.ConfigurationOptionProvider;
2324
import org.stagemonitor.configuration.ConfigurationRegistry;
2425
import org.stagemonitor.configuration.source.SimpleSource;
@@ -48,4 +49,10 @@ public static ConfigurationRegistry createSpyConfig() {
4849
.addConfigSource(new SimpleSource(CONFIG_SOURCE_NAME).add("service_name", "elastic-apm-test"))
4950
.build();
5051
}
52+
53+
public static void reset(ConfigurationRegistry config) {
54+
for (ConfigurationOptionProvider provider : config.getConfigurationOptionProviders()) {
55+
Mockito.reset(provider);
56+
}
57+
}
5158
}

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

Lines changed: 37 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
* Licensed under the Apache License, Version 2.0 (the "License");
88
* you may not use this file except in compliance with the License.
99
* You may obtain a copy of the License at
10-
*
10+
*
1111
* http://www.apache.org/licenses/LICENSE-2.0
12-
*
12+
*
1313
* Unless required by applicable law or agreed to in writing, software
1414
* distributed under the License is distributed on an "AS IS" BASIS,
1515
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -54,18 +54,37 @@ static void init(ElasticApmTracer tracer) {
5454

5555
@Nullable
5656
@Advice.OnMethodEnter
57-
public static Transaction onEnterServletService(@Advice.Argument(0) ServletRequest request) {
57+
public static Transaction onEnterServletService(@Advice.Argument(0) ServletRequest servletRequest) {
5858
if (servletTransactionHelper != null &&
59-
request instanceof HttpServletRequest &&
60-
!Boolean.TRUE.equals(request.getAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST))) {
61-
final HttpServletRequest httpServletRequest = (HttpServletRequest) request;
62-
final Transaction transaction = servletTransactionHelper.onBefore(httpServletRequest.getServletPath(), httpServletRequest.getPathInfo(),
63-
httpServletRequest.getRequestURI(), httpServletRequest.getHeader("User-Agent"),
64-
httpServletRequest.getHeader(TraceContext.TRACE_PARENT_HEADER));
59+
servletRequest instanceof HttpServletRequest &&
60+
!Boolean.TRUE.equals(servletRequest.getAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST))) {
61+
62+
final HttpServletRequest request = (HttpServletRequest) servletRequest;
63+
final Transaction transaction = servletTransactionHelper.onBefore(
64+
request.getServletPath(), request.getPathInfo(),
65+
request.getRequestURI(), request.getHeader("User-Agent"),
66+
request.getHeader(TraceContext.TRACE_PARENT_HEADER));
6567
if (transaction == null) {
6668
// if the request is excluded, avoid matching all exclude patterns again on each filter invocation
67-
httpServletRequest.setAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST, Boolean.TRUE);
69+
request.setAttribute(FilterChainInstrumentation.EXCLUDE_REQUEST, Boolean.TRUE);
70+
return null;
71+
}
72+
final Request req = transaction.getContext().getRequest();
73+
if (transaction.isSampled() && request.getCookies() != null) {
74+
for (Cookie cookie : request.getCookies()) {
75+
req.addCookie(cookie.getName(), cookie.getValue());
76+
}
77+
}
78+
final Enumeration headerNames = request.getHeaderNames();
79+
while (headerNames.hasMoreElements()) {
80+
final String headerName = (String) headerNames.nextElement();
81+
req.addHeader(headerName, request.getHeader(headerName));
6882
}
83+
84+
final Principal userPrincipal = request.getUserPrincipal();
85+
servletTransactionHelper.fillRequestContext(transaction, userPrincipal != null ? userPrincipal.getName() : null,
86+
request.getProtocol(), request.getMethod(), request.isSecure(), request.getScheme(), request.getServerName(),
87+
request.getServerPort(), request.getRequestURI(), request.getQueryString(), request.getRemoteAddr(), request.getRequestURL());
6988
return transaction;
7089
}
7190
return null;
@@ -76,30 +95,20 @@ public static void onExitServletService(@Advice.Argument(0) ServletRequest servl
7695
@Advice.Argument(1) ServletResponse servletResponse,
7796
@Advice.Enter @Nullable Transaction transaction,
7897
@Advice.Thrown @Nullable Exception exception) {
79-
if (servletTransactionHelper != null && transaction != null && servletRequest instanceof HttpServletRequest &&
98+
if (servletTransactionHelper != null &&
99+
transaction != null &&
100+
servletRequest instanceof HttpServletRequest &&
80101
servletResponse instanceof HttpServletResponse) {
81-
final HttpServletRequest request = (HttpServletRequest) servletRequest;
102+
82103
final HttpServletResponse response = (HttpServletResponse) servletResponse;
83-
final Request req = transaction.getContext().getRequest();
84-
if (request.getCookies() != null) {
85-
for (Cookie cookie : request.getCookies()) {
86-
req.addCookie(cookie.getName(), cookie.getValue());
87-
}
88-
}
89-
final Enumeration headerNames = request.getHeaderNames();
90-
while (headerNames.hasMoreElements()) {
91-
final String headerName = (String) headerNames.nextElement();
92-
req.addHeader(headerName, request.getHeader(headerName));
93-
}
104+
final HttpServletRequest request = (HttpServletRequest) servletRequest;
94105
final Response resp = transaction.getContext().getResponse();
95106
for (String headerName : response.getHeaderNames()) {
96107
resp.addHeader(headerName, response.getHeader(headerName));
97108
}
98-
final Principal userPrincipal = request.getUserPrincipal();
99-
servletTransactionHelper.onAfter(transaction, exception, userPrincipal != null ? userPrincipal.getName() : null,
100-
request.getProtocol(), request.getMethod(), request.isSecure(), request.getScheme(), request.getServerName(),
101-
request.getServerPort(), request.getRequestURI(), request.getQueryString(), request.getParameterMap(),
102-
request.getRemoteAddr(), request.getRequestURL(), response.isCommitted(), response.getStatus());
109+
110+
servletTransactionHelper.onAfter(transaction, exception, response.isCommitted(), response.getStatus(), request.getMethod(),
111+
request.getParameterMap());
103112
}
104113
}
105114
}

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

Lines changed: 65 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,29 @@ public class ServletTransactionHelper {
6363
this.webConfiguration = tracer.getConfig(WebConfiguration.class);
6464
}
6565

66+
/*
67+
* As much of the request information as possible should be set before the request processing starts.
68+
*
69+
* That way, when recording an error,
70+
* we can copy the transaction context to the error context.
71+
*
72+
* This has the advantage that we don't have to create the context for the error again.
73+
* As creating the context is framework specific,
74+
* this also means less effort when adding support for new frameworks,
75+
* because the creating the context is handled in one central place.
76+
*
77+
* Furthermore, it is not trivial to create an error context at an arbitrary location
78+
* (when the user calls ElasticApm.captureException()),
79+
* as we don't necessarily have access to the framework's request and response objects.
80+
*
81+
* Additionally, we only have access to the classes of the instrumented classes inside advice methods.
82+
*
83+
* Currently, there is no configuration option to disable tracing but to still enable error tracking.
84+
* But even when introducing that, the approach of copying the transaction context can still work.
85+
* We will then capture the transaction but not report it.
86+
* As the capturing of the transaction is garbage free, this should not add a significant overhead.
87+
* Also, this setting would be rather niche, as we are a APM solution after all.
88+
*/
6689
@Nullable
6790
@VisibleForAdvice
6891
public Transaction onBefore(String servletPath, String pathInfo, String requestURI,
@@ -78,34 +101,31 @@ public Transaction onBefore(String servletPath, String pathInfo, String requestU
78101
}
79102
}
80103

81-
/*
82-
* filling the transaction after the request has been processed is safer
83-
* as reading the parameters could potentially decode them in the wrong encoding
84-
* or trigger exceptions,
85-
* for example when the amount of query parameters is longer than the application server allows
86-
* in that case, we rather want that the agent looks like the cause for this
87-
*/
88104
@VisibleForAdvice
89-
public void onAfter(Transaction transaction, @Nullable Exception exception,
90-
@Nullable String userName, String protocol, String method, boolean secure, String scheme, String serverName,
91-
int serverPort, String requestURI, String queryString, Map<String, String[]> parameterMap, String remoteAddr,
92-
StringBuffer requestURL, boolean committed, int status) {
93-
try {
94-
Context context = transaction.getContext();
95-
final Request request = transaction.getContext().getRequest();
96-
fillRequest(request, protocol, method, secure, scheme, serverName, serverPort, requestURI, queryString, parameterMap,
97-
remoteAddr, requestURL);
98-
99-
fillResponse(context.getResponse(), committed, status);
100-
// only set username if not manually set
101-
if (context.getUser().getUsername() == null) {
102-
context.getUser().withUsername(userName);
103-
}
105+
public void fillRequestContext(Transaction transaction, @Nullable String userName, String protocol, String method, boolean secure,
106+
String scheme, String serverName, int serverPort, String requestURI, String queryString,
107+
String remoteAddr, StringBuffer requestURL) {
108+
// the HTTP method is not a good transaction name, but better than none...
109+
if (transaction.getName().length() == 0) {
110+
transaction.withName(method);
111+
}
112+
Context context = transaction.getContext();
113+
final Request request = transaction.getContext().getRequest();
114+
fillRequest(request, protocol, method, secure, scheme, serverName, serverPort, requestURI, queryString, remoteAddr, requestURL);
104115

105-
// the HTTP method is not a good transaction name, but better than none...
106-
if (transaction.getName().length() == 0) {
107-
transaction.withName(method);
108-
}
116+
// only set username if not manually set
117+
if (context.getUser().getUsername() == null) {
118+
context.getUser().withUsername(userName);
119+
}
120+
}
121+
122+
123+
@VisibleForAdvice
124+
public void onAfter(Transaction transaction, @Nullable Exception exception, boolean committed, int status, String method,
125+
Map<String, String[]> parameterMap) {
126+
try {
127+
fillRequestParameters(transaction, method, parameterMap);
128+
fillResponse(transaction.getContext().getResponse(), committed, status);
109129
transaction.withResult(ResultUtil.getResultByHttpStatus(status));
110130
transaction.withType("request");
111131
if (exception != null) {
@@ -118,6 +138,24 @@ public void onAfter(Transaction transaction, @Nullable Exception exception,
118138
transaction.end();
119139
}
120140

141+
/*
142+
* Filling the parameter after the request has been processed is safer
143+
* as reading the parameters could potentially decode them in the wrong encoding
144+
* or trigger exceptions,
145+
* for example when the amount of query parameters is longer than the application server allows.
146+
* In that case, we rather not want that the agent looks like the cause for this.
147+
*/
148+
private void fillRequestParameters(Transaction transaction, String method, Map<String, String[]> parameterMap) {
149+
Request request = transaction.getContext().getRequest();
150+
if (hasBody(request.getHeaders(), method)) {
151+
if (webConfiguration.getCaptureBody() != OFF) {
152+
captureBody(request, parameterMap);
153+
} else {
154+
request.redactBody();
155+
}
156+
}
157+
}
158+
121159
private boolean isExcluded(String servletPath, String pathInfo, String requestURI, @Nullable String userAgentHeader) {
122160
boolean excludeUrl = WildcardMatcher.anyMatch(webConfiguration.getIgnoreUrls(), servletPath, pathInfo);
123161
if (excludeUrl) {
@@ -140,16 +178,8 @@ private void fillResponse(Response response, boolean committed, int status) {
140178

141179
private void fillRequest(Request request, String protocol, String method, boolean secure,
142180
String scheme, String serverName, int serverPort, String requestURI, String queryString,
143-
Map<String, String[]> parameterMap, String remoteAddr, StringBuffer requestURL) {
144-
final WebConfiguration.EventType eventType = webConfiguration.getCaptureBody();
181+
String remoteAddr, StringBuffer requestURL) {
145182

146-
if (hasBody(request.getHeaders(), method)) {
147-
if (eventType != OFF) {
148-
captureBody(request, parameterMap);
149-
} else {
150-
request.redactBody();
151-
}
152-
}
153183
request.withHttpVersion(getHttpVersion(protocol));
154184
request.withMethod(method);
155185

0 commit comments

Comments
 (0)