Skip to content

Commit 607e6d1

Browse files
zUniQueXzUniQueX
authored
Fix metrics for InstrumentedEE10Handler (#3928)
Fixes #3917
1 parent 9e0bedb commit 607e6d1

File tree

8 files changed

+343
-120
lines changed

8 files changed

+343
-120
lines changed

‎metrics-jetty12-ee10/pom.xml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
<dependency>
8686
<groupId>org.eclipse.jetty</groupId>
8787
<artifactId>jetty-util</artifactId>
88+
<scope>provided</scope>
8889
</dependency>
8990
<dependency>
9091
<groupId>org.eclipse.jetty.ee10</groupId>
@@ -134,5 +135,11 @@
134135
<version>${slf4j.version}</version>
135136
<scope>test</scope>
136137
</dependency>
138+
<dependency>
139+
<groupId>org.awaitility</groupId>
140+
<artifactId>awaitility</artifactId>
141+
<version>${awaitility.version}</version>
142+
<scope>test</scope>
143+
</dependency>
137144
</dependencies>
138145
</project>

‎metrics-jetty12-ee10/src/main/java/io/dropwizard/metrics/jetty12/ee10/InstrumentedEE10Handler.java‎

Lines changed: 58 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,23 @@
11
package io.dropwizard.metrics.jetty12.ee10;
22

3+
import com.codahale.metrics.Meter;
34
import com.codahale.metrics.MetricRegistry;
45
import com.codahale.metrics.annotation.ResponseMeteredLevel;
56
import io.dropwizard.metrics.jetty12.AbstractInstrumentedHandler;
67
import jakarta.servlet.AsyncEvent;
78
import jakarta.servlet.AsyncListener;
8-
import org.eclipse.jetty.ee10.servlet.AsyncContextState;
9+
import jakarta.servlet.ServletRequest;
10+
import jakarta.servlet.ServletRequestEvent;
11+
import jakarta.servlet.ServletRequestListener;
912
import org.eclipse.jetty.ee10.servlet.ServletApiRequest;
10-
import org.eclipse.jetty.ee10.servlet.ServletApiResponse;
1113
import org.eclipse.jetty.ee10.servlet.ServletChannelState;
14+
import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
1215
import org.eclipse.jetty.ee10.servlet.ServletContextRequest;
1316
import org.eclipse.jetty.server.Handler;
1417
import org.eclipse.jetty.server.Request;
1518
import org.eclipse.jetty.server.Response;
16-
import org.eclipse.jetty.util.Callback;
1719

1820
import java.io.IOException;
19-
import java.util.concurrent.TimeUnit;
20-
21-
import static com.codahale.metrics.annotation.ResponseMeteredLevel.COARSE;
2221

2322
/**
2423
* A Jetty {@link Handler} which records various metrics about an underlying {@link Handler}
@@ -28,15 +27,15 @@
2827
* {@link org.eclipse.jetty.ee10.servlet.ServletContextHandler#insertHandler(Singleton)}.
2928
*/
3029
public class InstrumentedEE10Handler extends AbstractInstrumentedHandler {
31-
private AsyncListener listener;
30+
private AsyncDispatchesAwareServletRequestListener asyncDispatchesAwareServletRequestListener;
3231

3332
/**
3433
* Create a new instrumented handler using a given metrics registry.
3534
*
3635
* @param registry the registry for the metrics
3736
*/
3837
public InstrumentedEE10Handler(MetricRegistry registry) {
39-
super(registry, null);
38+
super(registry);
4039
}
4140

4241
/**
@@ -46,7 +45,7 @@ public InstrumentedEE10Handler(MetricRegistry registry) {
4645
* @param prefix the prefix to use for the metrics names
4746
*/
4847
public InstrumentedEE10Handler(MetricRegistry registry, String prefix) {
49-
super(registry, prefix, COARSE);
48+
super(registry, prefix);
5049
}
5150

5251
/**
@@ -63,8 +62,7 @@ public InstrumentedEE10Handler(MetricRegistry registry, String prefix, ResponseM
6362
@Override
6463
protected void doStart() throws Exception {
6564
super.doStart();
66-
67-
this.listener = new AsyncAttachingListener();
65+
asyncDispatchesAwareServletRequestListener = new AsyncDispatchesAwareServletRequestListener(getAsyncDispatches());
6866
}
6967

7068
@Override
@@ -73,104 +71,84 @@ protected void doStop() throws Exception {
7371
}
7472

7573
@Override
76-
public boolean handle(Request request, Response response, Callback callback) throws Exception {
74+
protected void setupServletListeners(Request request, Response response) {
7775
ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
78-
79-
// only handle servlet requests with the InstrumentedHandler
80-
// because it depends on the ServletRequestState
8176
if (servletContextRequest == null) {
82-
return super.handle(request, response, callback);
83-
}
84-
85-
activeDispatches.inc();
86-
87-
final long start;
88-
final ServletChannelState state = servletContextRequest.getServletRequestState();
89-
if (state.isInitial()) {
90-
// new request
91-
activeRequests.inc();
92-
start = Request.getTimeStamp(request);
93-
state.addListener(listener);
94-
} else {
95-
// resumed request
96-
start = System.currentTimeMillis();
97-
activeSuspended.dec();
98-
if (state.getState() == ServletChannelState.State.HANDLING) {
99-
asyncDispatches.mark();
100-
}
77+
return;
10178
}
10279

103-
boolean handled = false;
80+
ServletChannelState servletChannelState = servletContextRequest.getServletRequestState();
81+
// the ServletChannelState gets recycled after handling, so add a new listener for every request
82+
servletChannelState.addListener(new InstrumentedAsyncListener(getAsyncTimeouts()));
10483

105-
try {
106-
handled = super.handle(request, response, callback);
107-
} finally {
108-
final long now = System.currentTimeMillis();
109-
final long dispatched = now - start;
84+
ServletContextHandler servletContextHandler = servletContextRequest.getServletContextHandler();
85+
// addEventListener checks for duplicates, so we can try to add the listener for every request
86+
servletContextHandler.addEventListener(asyncDispatchesAwareServletRequestListener);
87+
}
11088

111-
activeDispatches.dec();
112-
dispatches.update(dispatched, TimeUnit.MILLISECONDS);
89+
@Override
90+
protected boolean isSuspended(Request request, Response response) {
91+
ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class);
92+
if (servletContextRequest == null) {
93+
return false;
94+
}
11395

114-
if (state.isSuspended()) {
115-
activeSuspended.inc();
116-
} else if (state.isInitial()) {
117-
updateResponses(request, response, start, handled);
118-
}
119-
// else onCompletion will handle it.
96+
ServletChannelState servletChannelState = servletContextRequest.getServletRequestState();
97+
if (servletChannelState == null) {
98+
return false;
12099
}
121100

122-
return handled;
101+
return servletChannelState.isSuspended();
123102
}
124103

125-
private class AsyncAttachingListener implements AsyncListener {
126-
127-
@Override
128-
public void onTimeout(AsyncEvent event) throws IOException {}
104+
private static class AsyncDispatchesAwareServletRequestListener implements ServletRequestListener {
105+
private final Meter asyncDispatches;
129106

130-
@Override
131-
public void onStartAsync(AsyncEvent event) throws IOException {
132-
event.getAsyncContext().addListener(new InstrumentedAsyncListener());
107+
private AsyncDispatchesAwareServletRequestListener(Meter asyncDispatches) {
108+
this.asyncDispatches = asyncDispatches;
133109
}
134110

135111
@Override
136-
public void onError(AsyncEvent event) throws IOException {}
112+
public void requestInitialized(ServletRequestEvent sre) {
113+
ServletRequest servletRequest = sre.getServletRequest();
114+
if (!(servletRequest instanceof ServletApiRequest)) {
115+
return;
116+
}
137117

138-
@Override
139-
public void onComplete(AsyncEvent event) throws IOException {}
118+
ServletApiRequest servletApiRequest = (ServletApiRequest) servletRequest;
119+
120+
ServletContextHandler.ServletRequestInfo servletRequestInfo = servletApiRequest.getServletRequestInfo();
121+
122+
ServletChannelState servletChannelState = servletRequestInfo.getServletRequestState();
123+
124+
// if the request isn't 'initial', the request was re-dispatched
125+
if (servletChannelState.isAsync() && !servletChannelState.isInitial()) {
126+
asyncDispatches.mark();
127+
}
128+
}
140129
}
141130

142-
private class InstrumentedAsyncListener implements AsyncListener {
143-
private final long startTime;
131+
private static class InstrumentedAsyncListener implements AsyncListener {
132+
private final Meter asyncTimeouts;
144133

145-
InstrumentedAsyncListener() {
146-
this.startTime = System.currentTimeMillis();
134+
private InstrumentedAsyncListener(Meter asyncTimeouts) {
135+
this.asyncTimeouts = asyncTimeouts;
147136
}
148137

149138
@Override
150-
public void onTimeout(AsyncEvent event) throws IOException {
151-
asyncTimeouts.mark();
152-
}
139+
public void onComplete(AsyncEvent event) throws IOException {}
153140

154141
@Override
155-
public void onStartAsync(AsyncEvent event) throws IOException {
142+
public void onTimeout(AsyncEvent event) throws IOException {
143+
asyncTimeouts.mark();
156144
}
157145

158146
@Override
159-
public void onError(AsyncEvent event) throws IOException {
160-
}
147+
public void onError(AsyncEvent event) throws IOException {}
161148

162149
@Override
163-
public void onComplete(AsyncEvent event) throws IOException {
164-
final AsyncContextState state = (AsyncContextState) event.getAsyncContext();
165-
final ServletApiRequest request = (ServletApiRequest) state.getRequest();
166-
final ServletApiResponse response = (ServletApiResponse) state.getResponse();
167-
updateResponses(request.getRequest(), response.getResponse(), startTime, true);
168-
169-
final ServletContextRequest servletContextRequest = Request.as(request.getRequest(), ServletContextRequest.class);
170-
final ServletChannelState servletRequestState = servletContextRequest.getServletRequestState();
171-
if (!servletRequestState.isSuspended()) {
172-
activeSuspended.dec();
173-
}
150+
public void onStartAsync(AsyncEvent event) throws IOException {
151+
event.getAsyncContext().addListener(this);
174152
}
175153
}
176154
}

‎metrics-jetty12-ee10/src/test/java/io/dropwizard/metrics/jetty12/ee10/AbstractIntegrationTest.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package io.dropwizard.metrics.jetty12.ee10;
2+
3+
import com.codahale.metrics.MetricRegistry;
4+
import org.eclipse.jetty.client.HttpClient;
5+
import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
6+
import org.eclipse.jetty.server.Handler;
7+
import org.eclipse.jetty.server.Server;
8+
import org.eclipse.jetty.server.ServerConnector;
9+
import org.junit.After;
10+
import org.junit.Before;
11+
12+
import static com.codahale.metrics.annotation.ResponseMeteredLevel.ALL;
13+
14+
abstract class AbstractIntegrationTest {
15+
16+
protected final HttpClient client = new HttpClient();
17+
protected final MetricRegistry registry = new MetricRegistry();
18+
protected final Server server = new Server();
19+
protected final ServerConnector connector = new ServerConnector(server);
20+
protected final InstrumentedEE10Handler handler = new InstrumentedEE10Handler(registry, null, ALL);
21+
protected final ServletContextHandler servletContextHandler = new ServletContextHandler();
22+
23+
@Before
24+
public void setUp() throws Exception {
25+
handler.setName("handler");
26+
27+
// builds the following handler chain:
28+
// ServletContextHandler -> InstrumentedHandler -> TestHandler
29+
// the ServletContextHandler is needed to utilize servlet related classes
30+
servletContextHandler.setHandler(getHandler());
31+
servletContextHandler.insertHandler(handler);
32+
server.setHandler(servletContextHandler);
33+
34+
server.addConnector(connector);
35+
server.start();
36+
client.start();
37+
}
38+
39+
@After
40+
public void tearDown() throws Exception {
41+
server.stop();
42+
client.stop();
43+
}
44+
45+
protected String uri(String path) {
46+
return "http://localhost:" + connector.getLocalPort() + path;
47+
}
48+
49+
protected abstract Handler getHandler();
50+
}

‎metrics-jetty12-ee10/src/test/java/io/dropwizard/metrics/jetty12/ee10/AsyncTest.java‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package io.dropwizard.metrics.jetty12.ee10;
2+
3+
import com.codahale.metrics.Counter;
4+
import com.codahale.metrics.Meter;
5+
import com.codahale.metrics.MetricRegistry;
6+
import jakarta.servlet.AsyncContext;
7+
import jakarta.servlet.DispatcherType;
8+
import jakarta.servlet.http.HttpServletRequest;
9+
import jakarta.servlet.http.HttpServletResponse;
10+
import org.eclipse.jetty.client.CompletableResponseListener;
11+
import org.eclipse.jetty.client.ContentResponse;
12+
import org.eclipse.jetty.client.Request;
13+
import org.eclipse.jetty.client.Response;
14+
import org.eclipse.jetty.ee10.servlet.ServletHandler;
15+
import org.eclipse.jetty.server.Handler;
16+
import org.junit.Test;
17+
18+
import java.util.EnumSet;
19+
import java.util.concurrent.CompletableFuture;
20+
import java.util.concurrent.TimeUnit;
21+
22+
import static org.assertj.core.api.Assertions.assertThat;
23+
import static org.assertj.core.api.Assertions.assertThatNoException;
24+
import static org.awaitility.Awaitility.await;
25+
26+
public class AsyncTest extends AbstractIntegrationTest {
27+
28+
@Override
29+
protected Handler getHandler() {
30+
return new ServletHandler();
31+
}
32+
33+
@Test
34+
public void testAsyncTimeout() throws Exception {
35+
servletContextHandler.addFilter((request, response, chain) -> {
36+
AsyncContext asyncContext = request.startAsync();
37+
asyncContext.setTimeout(1);
38+
}, "/*", EnumSet.allOf(DispatcherType.class));
39+
40+
client.GET(uri("/"));
41+
Meter asyncTimeouts = registry.meter(MetricRegistry.name(ServletHandler.class, "handler.async-timeouts"));
42+
assertThat(asyncTimeouts.getCount()).isEqualTo(1L);
43+
44+
client.GET(uri("/"));
45+
assertThat(asyncTimeouts.getCount()).isEqualTo(2L);
46+
}
47+
48+
@Test
49+
public void testActiveSuspended() {
50+
servletContextHandler.addFilter((request, response, chain) -> {
51+
AsyncContext asyncContext = request.startAsync();
52+
asyncContext.start(() -> {
53+
try {
54+
Thread.sleep(1000);
55+
} catch (InterruptedException interruptedException) {
56+
Thread.currentThread().interrupt();
57+
}
58+
asyncContext.complete();
59+
});
60+
}, "/*", EnumSet.allOf(DispatcherType.class));
61+
62+
Counter activeSuspended = registry.counter(MetricRegistry.name(ServletHandler.class, "handler.active-suspended"));
63+
Request request = client.POST(uri("/"));
64+
CompletableResponseListener completableResponseListener = new CompletableResponseListener(request);
65+
CompletableFuture<ContentResponse> asyncResponse = completableResponseListener.send();
66+
assertThatNoException().isThrownBy(() -> {
67+
await()
68+
.atMost(750, TimeUnit.MILLISECONDS)
69+
.until(() -> activeSuspended.getCount() == 1L);
70+
asyncResponse.get();
71+
});
72+
assertThat(activeSuspended.getCount()).isEqualTo(0L);
73+
}
74+
75+
@Test
76+
public void testAsyncDispatches() throws Exception {
77+
servletContextHandler.addFilter((request, response, chain) -> {
78+
if (!(request instanceof HttpServletRequest)) {
79+
throw new IllegalStateException("Expecting ServletRequest to be an instance of HttpServletRequest");
80+
}
81+
HttpServletRequest httpServletRequest = (HttpServletRequest) request;
82+
if ("/".equals(httpServletRequest.getRequestURI())) {
83+
AsyncContext asyncContext = request.startAsync();
84+
asyncContext.dispatch("/dispatch");
85+
return;
86+
}
87+
if ("/dispatch".equals(httpServletRequest.getRequestURI())) {
88+
AsyncContext asyncContext = request.startAsync();
89+
if (!(response instanceof HttpServletResponse)) {
90+
throw new IllegalStateException("Expecting ServletResponse to be an instance of HttpServletResponse");
91+
}
92+
HttpServletResponse httpServletResponse = (HttpServletResponse) response;
93+
httpServletResponse.setStatus(204);
94+
asyncContext.complete();
95+
return;
96+
}
97+
throw new UnsupportedOperationException("Only '/' and '/dispatch' are valid paths");
98+
}, "/*", EnumSet.allOf(DispatcherType.class));
99+
100+
ContentResponse contentResponse = client.GET(uri("/"));
101+
assertThat(contentResponse).isNotNull().extracting(Response::getStatus).isEqualTo(204);
102+
Meter asyncDispatches = registry.meter(MetricRegistry.name(ServletHandler.class, "handler.async-dispatches"));
103+
assertThat(asyncDispatches.getCount()).isEqualTo(1L);
104+
}
105+
}

0 commit comments

Comments
 (0)