Skip to content

Avoid setting request attributes to reduce allocations #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 27, 2018
Merged

Avoid setting request attributes to reduce allocations #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f0eabd
Avoid setting request attributes to reduce allocations
felixbarny Nov 17, 2018
9996cae
Merge branch 'master' into avoid-request-set-attribute
felixbarny Nov 26, 2018
17c16bd
Fix Java 7 related compile error
felixbarny Nov 26, 2018
6650794
Test that excluded URLs are only matched once
felixbarny Nov 27, 2018
b95f749
Merge remote-tracking branch 'origin/master' into avoid-request-set-a…
felixbarny Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions ...plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/AsyncInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -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);
}
}
}
}
}
Expand Down
4 changes: 0 additions & 4 deletions ...s/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/FilterChainInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
14 changes: 10 additions & 4 deletions ...ent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletApiAdvice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand All @@ -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) {
Expand Down Expand Up @@ -121,6 +126,7 @@ public static void onExitServletService(@Advice.Argument(0) ServletRequest servl
if (tracer == null) {
return;
}
excluded.set(Boolean.FALSE);
Copy link
Contributor

@eyalkoren eyalkoren Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the sake of accurate state management, this should be done only when transaction != null.

Copy link
Member Author

@felixbarny felixbarny Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transaction is null for excluded requests which is the exact case where we do want to set excluded back to the initial value. We could set it to false only if it's true or if transaction is null but I think it's simpler and not necessarily slower to set it always to false, just to be sure we don't miss anything.

Copy link
Contributor

@eyalkoren eyalkoren Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of nested service calls, you set the excluded to true in the outmost invocation, so you should reset it in the exit of the same method. I understand why transaction != null is not good though as you say.
So what I suggest is: if you DO care about the state being set and reset accurately in the case of nested calls, you can add a local boolean variable that says whether this specific service method was the one changing the state or not. However, this is not super important I assume, so I am fine with whatever you decide.

Copy link
Member Author

@felixbarny felixbarny Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of nested service calls, you set the excluded to true in the outmost invocation, so you should reset it in the exit of the same method.

I think it's just fine to set it to false in all nested filters and servlets. If it has already been reset to false, resetting it again does not change the state.

if (scope != null) {
scope.close();
}
Expand Down
11 changes: 7 additions & 4 deletions ...ins/apm-servlet-plugin/src/main/java/co/elastic/apm/servlet/ServletTransactionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 5 additions & 0 deletions apm-agent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/servlet/ApmFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}

Expand Down