Skip to content

Commit b3273de

Browse files
pavolloffaypavolloffay
authored
Dynamically disable servlet instrumentation if class cast exception happens (#138)
Signed-off-by: Pavol Loffay <[email protected]>
1 parent ca167d3 commit b3273de

File tree

12 files changed

+517
-168
lines changed

12 files changed

+517
-168
lines changed

Diff for: ‎instrumentation/servlet/servlet-2.3/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/Servlet2BodyInstrumentationModule.java

+45-35
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public static class Filter2Advice {
110110
public static boolean start(
111111
@Advice.Argument(value = 0, readOnly = false) ServletRequest request,
112112
@Advice.Argument(value = 1, readOnly = false) ServletResponse response,
113-
@Advice.Local("rootStart") Boolean rootStart) {
113+
@Advice.Local("rootStart") boolean rootStart) {
114114

115115
if (!HypertraceConfig.isInstrumentationEnabled(
116116
Servlet2InstrumentationName.PRIMARY, Servlet2InstrumentationName.OTHER)) {
@@ -134,8 +134,11 @@ public static boolean start(
134134
Span currentSpan = Java8BytecodeBridge.currentSpan();
135135

136136
rootStart = true;
137-
response = new BufferingHttpServletResponse(httpResponse);
138-
request = new BufferingHttpServletRequest(httpRequest, (HttpServletResponse) response);
137+
138+
if (!HypertraceConfig.disableServletWrapperTypes()) {
139+
response = new BufferingHttpServletResponse(httpResponse);
140+
request = new BufferingHttpServletRequest(httpRequest, (HttpServletResponse) response);
141+
}
139142

140143
ServletSpanDecorator.addSessionId(currentSpan, httpRequest);
141144

@@ -165,41 +168,48 @@ public static boolean start(
165168
public static void stopSpan(
166169
@Advice.Argument(0) ServletRequest request,
167170
@Advice.Argument(1) ServletResponse response,
168-
@Advice.Local("rootStart") Boolean rootStart) {
169-
if (rootStart != null) {
170-
if (!(request instanceof BufferingHttpServletRequest)
171-
|| !(response instanceof BufferingHttpServletResponse)) {
172-
return;
173-
}
171+
@Advice.Thrown Throwable throwable,
172+
@Advice.Local("rootStart") boolean rootStart) {
173+
174+
HypertraceConfig.recordException(throwable);
175+
176+
if (!rootStart
177+
|| !(request instanceof BufferingHttpServletRequest)
178+
|| !(response instanceof BufferingHttpServletResponse)) {
179+
return;
180+
}
181+
182+
if (!(request instanceof BufferingHttpServletRequest)
183+
|| !(response instanceof BufferingHttpServletResponse)) {
184+
return;
185+
}
174186

175-
request.removeAttribute(ALREADY_LOADED);
176-
Span currentSpan = Java8BytecodeBridge.currentSpan();
177-
178-
BufferingHttpServletResponse bufferingResponse = (BufferingHttpServletResponse) response;
179-
BufferingHttpServletRequest bufferingRequest = (BufferingHttpServletRequest) request;
180-
181-
if (HypertraceConfig.get().getDataCapture().getHttpHeaders().getResponse().getValue()) {
182-
// set response headers
183-
Map<String, List<String>> bufferedHeaders = bufferingResponse.getBufferedHeaders();
184-
for (Map.Entry<String, List<String>> nameToHeadersEntry : bufferedHeaders.entrySet()) {
185-
String headerName = nameToHeadersEntry.getKey();
186-
for (String headerValue : nameToHeadersEntry.getValue()) {
187-
currentSpan.setAttribute(
188-
HypertraceSemanticAttributes.httpResponseHeader(headerName), headerValue);
189-
}
187+
request.removeAttribute(ALREADY_LOADED);
188+
Span currentSpan = Java8BytecodeBridge.currentSpan();
189+
190+
BufferingHttpServletResponse bufferingResponse = (BufferingHttpServletResponse) response;
191+
BufferingHttpServletRequest bufferingRequest = (BufferingHttpServletRequest) request;
192+
193+
if (HypertraceConfig.get().getDataCapture().getHttpHeaders().getResponse().getValue()) {
194+
// set response headers
195+
Map<String, List<String>> bufferedHeaders = bufferingResponse.getBufferedHeaders();
196+
for (Map.Entry<String, List<String>> nameToHeadersEntry : bufferedHeaders.entrySet()) {
197+
String headerName = nameToHeadersEntry.getKey();
198+
for (String headerValue : nameToHeadersEntry.getValue()) {
199+
currentSpan.setAttribute(
200+
HypertraceSemanticAttributes.httpResponseHeader(headerName), headerValue);
190201
}
191202
}
192-
// Bodies are captured at the end after all user processing.
193-
if (HypertraceConfig.get().getDataCapture().getHttpBody().getRequest().getValue()) {
194-
currentSpan.setAttribute(
195-
HypertraceSemanticAttributes.HTTP_REQUEST_BODY,
196-
bufferingRequest.getBufferedBodyAsString());
197-
}
198-
if (HypertraceConfig.get().getDataCapture().getHttpBody().getResponse().getValue()) {
199-
currentSpan.setAttribute(
200-
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY,
201-
bufferingResponse.getBufferAsString());
202-
}
203+
}
204+
// Bodies are captured at the end after all user processing.
205+
if (HypertraceConfig.get().getDataCapture().getHttpBody().getRequest().getValue()) {
206+
currentSpan.setAttribute(
207+
HypertraceSemanticAttributes.HTTP_REQUEST_BODY,
208+
bufferingRequest.getBufferedBodyAsString());
209+
}
210+
if (HypertraceConfig.get().getDataCapture().getHttpBody().getResponse().getValue()) {
211+
currentSpan.setAttribute(
212+
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, bufferingResponse.getBufferAsString());
203213
}
204214
}
205215
}

Diff for: ‎instrumentation/servlet/servlet-2.3/src/test/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/RequestCastServlet.java

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/*
2+
* Copyright The Hypertrace Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.opentelemetry.instrumentation.hypertrace.servlet.v2_3;
18+
19+
import javax.servlet.http.HttpServletRequest;
20+
import javax.servlet.http.HttpServletResponse;
21+
22+
public class RequestCastServlet extends TestServlet {
23+
24+
@Override
25+
protected void service(HttpServletRequest req, HttpServletResponse resp) {
26+
super.service(req, resp);
27+
org.eclipse.jetty.server.Request jettyRequest = (org.eclipse.jetty.server.Request) req;
28+
org.eclipse.jetty.server.Response jettyResponse = (org.eclipse.jetty.server.Response) resp;
29+
}
30+
}

Diff for: ‎instrumentation/servlet/servlet-2.3/src/test/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/Servlet23InstrumentationTest.java

+78-14
Original file line numberDiff line numberDiff line change
@@ -26,30 +26,41 @@
2626
import org.eclipse.jetty.servlet.ServletContextHandler;
2727
import org.hypertrace.agent.core.HypertraceSemanticAttributes;
2828
import org.hypertrace.agent.testing.AbstractInstrumenterTest;
29+
import org.junit.jupiter.api.AfterAll;
2930
import org.junit.jupiter.api.Assertions;
31+
import org.junit.jupiter.api.BeforeAll;
3032
import org.junit.jupiter.api.Test;
3133

3234
public class Servlet23InstrumentationTest extends AbstractInstrumenterTest {
35+
private static final String REQUEST_BODY = "hello";
36+
private static final String REQUEST_HEADER = "requestheader";
37+
private static final String REQUEST_HEADER_VALUE = "requestvalue";
3338

34-
@Test
35-
public void postJson() throws Exception {
36-
Server server = new Server(0);
39+
private static Server server = new Server(0);
40+
private static int serverPort;
41+
42+
@BeforeAll
43+
public static void startServer() throws Exception {
3744
ServletContextHandler handler = new ServletContextHandler();
3845
handler.addServlet(TestServlet.class, "/test");
39-
46+
handler.addServlet(RequestCastServlet.class, "/cast");
4047
server.setHandler(handler);
4148
server.start();
49+
serverPort = server.getConnectors()[0].getLocalPort();
50+
}
4251

43-
int serverPort = server.getConnectors()[0].getLocalPort();
52+
@AfterAll
53+
public static void stopServer() throws Exception {
54+
server.stop();
55+
}
4456

45-
String requestBody = "hello";
46-
String requestHeader = "requestheader";
47-
String requestHeaderValue = "requestvalue";
57+
@Test
58+
public void postJson() throws Exception {
4859
Request request =
4960
new Request.Builder()
5061
.url(String.format("http://localhost:%d/test", serverPort))
51-
.post(RequestBody.create(requestBody, MediaType.get("application/json")))
52-
.header(requestHeader, requestHeaderValue)
62+
.post(RequestBody.create(REQUEST_BODY, MediaType.get("application/json")))
63+
.header(REQUEST_HEADER, REQUEST_HEADER_VALUE)
5364
.build();
5465
try (Response response = httpClient.newCall(request).execute()) {
5566
Assertions.assertEquals(200, response.code());
@@ -63,12 +74,12 @@ public void postJson() throws Exception {
6374
Assertions.assertEquals(1, spans.size());
6475
SpanData spanData = spans.get(0);
6576
Assertions.assertEquals(
66-
requestBody, spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY));
77+
REQUEST_BODY, spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY));
6778
Assertions.assertEquals(
68-
requestHeaderValue,
79+
REQUEST_HEADER_VALUE,
6980
spanData
7081
.getAttributes()
71-
.get(HypertraceSemanticAttributes.httpRequestHeader(requestHeader)));
82+
.get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER)));
7283

7384
Assertions.assertEquals(
7485
TestServlet.RESPONSE_BODY,
@@ -78,6 +89,59 @@ public void postJson() throws Exception {
7889
spanData
7990
.getAttributes()
8091
.get(HypertraceSemanticAttributes.httpResponseHeader(TestServlet.RESPONSE_HEADER)));
81-
server.stop();
92+
}
93+
94+
@Test
95+
public void requestCast() throws Exception {
96+
Request request =
97+
new Request.Builder()
98+
.url(String.format("http://localhost:%d/cast", serverPort))
99+
.post(RequestBody.create(REQUEST_BODY, MediaType.get("application/json")))
100+
.header(REQUEST_HEADER, REQUEST_HEADER_VALUE)
101+
.build();
102+
try (Response response = httpClient.newCall(request).execute()) {
103+
Assertions.assertEquals(500, response.code());
104+
}
105+
TEST_WRITER.waitForTraces(1);
106+
List<List<SpanData>> traces = TEST_WRITER.getTraces();
107+
List<SpanData> spans = traces.get(0);
108+
Assertions.assertEquals(1, spans.size());
109+
SpanData spanData = spans.get(0);
110+
Assertions.assertEquals(
111+
REQUEST_HEADER_VALUE,
112+
spanData
113+
.getAttributes()
114+
.get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER)));
115+
Assertions.assertEquals(
116+
TestServlet.RESPONSE_HEADER_VALUE,
117+
spanData
118+
.getAttributes()
119+
.get(HypertraceSemanticAttributes.httpResponseHeader(TestServlet.RESPONSE_HEADER)));
120+
121+
TEST_WRITER.clear();
122+
try (Response response = httpClient.newCall(request).execute()) {
123+
Assertions.assertEquals(200, response.code());
124+
Assertions.assertEquals(TestServlet.RESPONSE_BODY, response.body().string());
125+
}
126+
traces = TEST_WRITER.getTraces();
127+
spans = traces.get(0);
128+
Assertions.assertEquals(1, spans.size());
129+
spanData = spans.get(0);
130+
Assertions.assertEquals(
131+
REQUEST_HEADER_VALUE,
132+
spanData
133+
.getAttributes()
134+
.get(HypertraceSemanticAttributes.httpRequestHeader(REQUEST_HEADER)));
135+
// response headers are captured in wrapped types
136+
// Assertions.assertEquals(
137+
// TestServlet.RESPONSE_HEADER_VALUE,
138+
// spanData
139+
// .getAttributes()
140+
//
141+
// .get(HypertraceSemanticAttributes.httpResponseHeader(TestServlet.RESPONSE_HEADER)));
142+
Assertions.assertNull(
143+
spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY));
144+
Assertions.assertNull(
145+
spanData.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY));
82146
}
83147
}

Diff for: ‎instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_0/Servlet30BodyInstrumentationModule.java

+58-44
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import net.bytebuddy.description.method.MethodDescription;
4646
import net.bytebuddy.description.type.TypeDescription;
4747
import net.bytebuddy.matcher.ElementMatcher;
48+
import org.hypertrace.agent.config.Config.AgentConfig;
4849
import org.hypertrace.agent.core.HypertraceConfig;
4950
import org.hypertrace.agent.core.HypertraceSemanticAttributes;
5051
import org.hypertrace.agent.filter.FilterRegistry;
@@ -105,7 +106,7 @@ public static class FilterAdvice {
105106
public static boolean start(
106107
@Advice.Argument(value = 0, readOnly = false) ServletRequest request,
107108
@Advice.Argument(value = 1, readOnly = false) ServletResponse response,
108-
@Advice.Local("rootStart") Boolean rootStart) {
109+
@Advice.Local("rootStart") boolean rootStart) {
109110

110111
if (!HypertraceConfig.isInstrumentationEnabled(
111112
Servlet30InstrumentationName.PRIMARY, Servlet30InstrumentationName.OTHER)) {
@@ -129,8 +130,10 @@ public static boolean start(
129130
Span currentSpan = Java8BytecodeBridge.currentSpan();
130131

131132
rootStart = true;
132-
response = new BufferingHttpServletResponse(httpResponse);
133-
request = new BufferingHttpServletRequest(httpRequest, (HttpServletResponse) response);
133+
if (!HypertraceConfig.disableServletWrapperTypes()) {
134+
response = new BufferingHttpServletResponse(httpResponse);
135+
request = new BufferingHttpServletRequest(httpRequest, (HttpServletResponse) response);
136+
}
134137

135138
ServletSpanDecorator.addSessionId(currentSpan, httpRequest);
136139

@@ -158,51 +161,62 @@ public static boolean start(
158161
public static void stopSpan(
159162
@Advice.Argument(0) ServletRequest request,
160163
@Advice.Argument(1) ServletResponse response,
161-
@Advice.Local("rootStart") Boolean rootStart) {
162-
if (rootStart != null) {
163-
if (!(request instanceof BufferingHttpServletRequest)
164-
|| !(response instanceof BufferingHttpServletResponse)) {
165-
return;
164+
@Advice.Thrown Throwable throwable,
165+
@Advice.Local("rootStart") boolean rootStart) {
166+
167+
HypertraceConfig.recordException(throwable);
168+
169+
if (!rootStart
170+
|| !(request instanceof HttpServletRequest)
171+
|| !(response instanceof HttpServletResponse)) {
172+
return;
173+
}
174+
175+
Span currentSpan = Java8BytecodeBridge.currentSpan();
176+
HttpServletResponse httpServletResponse = (HttpServletResponse) response;
177+
// set response headers
178+
AgentConfig agentConfig = HypertraceConfig.get();
179+
if (agentConfig.getDataCapture().getHttpHeaders().getResponse().getValue()) {
180+
for (String headerName : httpServletResponse.getHeaderNames()) {
181+
String headerValue = httpServletResponse.getHeader(headerName);
182+
currentSpan.setAttribute(
183+
HypertraceSemanticAttributes.httpResponseHeader(headerName), headerValue);
166184
}
185+
}
167186

168-
request.removeAttribute(ALREADY_LOADED);
169-
Span currentSpan = Java8BytecodeBridge.currentSpan();
170-
171-
AtomicBoolean responseHandled = new AtomicBoolean(false);
172-
if (request.isAsyncStarted()) {
173-
try {
174-
request
175-
.getAsyncContext()
176-
.addListener(new BodyCaptureAsyncListener(responseHandled, currentSpan));
177-
} catch (IllegalStateException e) {
178-
// org.eclipse.jetty.server.Request may throw an exception here if request became
179-
// finished after check above. We just ignore that exception and move on.
180-
}
187+
if (!(request instanceof BufferingHttpServletRequest)
188+
|| !(response instanceof BufferingHttpServletResponse)) {
189+
return;
190+
}
191+
192+
request.removeAttribute(ALREADY_LOADED);
193+
194+
AtomicBoolean responseHandled = new AtomicBoolean(false);
195+
if (request.isAsyncStarted()) {
196+
try {
197+
request
198+
.getAsyncContext()
199+
.addListener(new BodyCaptureAsyncListener(responseHandled, currentSpan));
200+
} catch (IllegalStateException e) {
201+
// org.eclipse.jetty.server.Request may throw an exception here if request became
202+
// finished after check above. We just ignore that exception and move on.
181203
}
204+
}
205+
206+
if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) {
207+
BufferingHttpServletResponse bufferingResponse = (BufferingHttpServletResponse) response;
208+
BufferingHttpServletRequest bufferingRequest = (BufferingHttpServletRequest) request;
182209

183-
if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) {
184-
BufferingHttpServletResponse bufferingResponse = (BufferingHttpServletResponse) response;
185-
BufferingHttpServletRequest bufferingRequest = (BufferingHttpServletRequest) request;
186-
187-
// set response headers
188-
if (HypertraceConfig.get().getDataCapture().getHttpHeaders().getResponse().getValue()) {
189-
for (String headerName : bufferingResponse.getHeaderNames()) {
190-
String headerValue = bufferingResponse.getHeader(headerName);
191-
currentSpan.setAttribute(
192-
HypertraceSemanticAttributes.httpResponseHeader(headerName), headerValue);
193-
}
194-
}
195-
// Bodies are captured at the end after all user processing.
196-
if (HypertraceConfig.get().getDataCapture().getHttpBody().getRequest().getValue()) {
197-
currentSpan.setAttribute(
198-
HypertraceSemanticAttributes.HTTP_REQUEST_BODY,
199-
bufferingRequest.getBufferedBodyAsString());
200-
}
201-
if (HypertraceConfig.get().getDataCapture().getHttpBody().getResponse().getValue()) {
202-
currentSpan.setAttribute(
203-
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY,
204-
bufferingResponse.getBufferAsString());
205-
}
210+
// Bodies are captured at the end after all user processing.
211+
if (HypertraceConfig.get().getDataCapture().getHttpBody().getRequest().getValue()) {
212+
currentSpan.setAttribute(
213+
HypertraceSemanticAttributes.HTTP_REQUEST_BODY,
214+
bufferingRequest.getBufferedBodyAsString());
215+
}
216+
if (HypertraceConfig.get().getDataCapture().getHttpBody().getResponse().getValue()) {
217+
currentSpan.setAttribute(
218+
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY,
219+
bufferingResponse.getBufferAsString());
206220
}
207221
}
208222
}

0 commit comments

Comments
 (0)