Skip to content

Commit 0dd2977

Browse files
pavolloffaypavolloffay
authored
Do not call get streams on request/response objects (#243)
* Do not call get streams on request/response objects Signed-off-by: Pavol Loffay <[email protected]> * Throwable check Signed-off-by: Pavol Loffay <[email protected]> * Handle response Signed-off-by: Pavol Loffay <[email protected]> * nice Signed-off-by: Pavol Loffay <[email protected]>
1 parent 1a7f158 commit 0dd2977

File tree

8 files changed

+235
-82
lines changed

8 files changed

+235
-82
lines changed

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/BodyCaptureAsyncListener.java

+19-8
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
import io.opentelemetry.api.trace.Span;
2020
import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
21+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.RequestStreamReaderHolder;
22+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ResponseStreamWriterHolder;
2123
import java.io.BufferedReader;
2224
import java.io.PrintWriter;
2325
import java.util.concurrent.atomic.AtomicBoolean;
@@ -42,27 +44,34 @@ public class BodyCaptureAsyncListener implements AsyncListener {
4244

4345
private final AtomicBoolean responseHandled;
4446
private final Span span;
47+
48+
private final ContextStore<HttpServletResponse, ResponseStreamWriterHolder> responseContextStore;
4549
private final ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> streamContextStore;
4650
private final ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore;
4751

48-
private final ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext;
49-
private final ContextStore<BufferedReader, CharBufferSpanPair> readerContext;
52+
private final ContextStore<HttpServletRequest, RequestStreamReaderHolder> requestContextStore;
53+
private final ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContextStore;
54+
private final ContextStore<BufferedReader, CharBufferSpanPair> readerContextStore;
5055

5156
private final AgentConfig agentConfig = HypertraceConfig.get();
5257

5358
public BodyCaptureAsyncListener(
5459
AtomicBoolean responseHandled,
5560
Span span,
61+
ContextStore<HttpServletResponse, ResponseStreamWriterHolder> responseContextStore,
5662
ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> streamContextStore,
5763
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore,
58-
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext,
59-
ContextStore<BufferedReader, CharBufferSpanPair> readerContext) {
64+
ContextStore<HttpServletRequest, RequestStreamReaderHolder> requestContextStore,
65+
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContextStore,
66+
ContextStore<BufferedReader, CharBufferSpanPair> readerContextStore) {
6067
this.responseHandled = responseHandled;
6168
this.span = span;
69+
this.responseContextStore = responseContextStore;
6270
this.streamContextStore = streamContextStore;
6371
this.writerContextStore = writerContextStore;
64-
this.inputStreamContext = inputStreamContext;
65-
this.readerContext = readerContext;
72+
this.requestContextStore = requestContextStore;
73+
this.inputStreamContextStore = inputStreamContextStore;
74+
this.readerContextStore = readerContextStore;
6675
}
6776

6877
@Override
@@ -94,7 +103,8 @@ private void captureResponseDataAndClearRequestBuffer(
94103

95104
if (agentConfig.getDataCapture().getHttpBody().getResponse().getValue()
96105
&& ContentTypeUtils.shouldCapture(httpResponse.getContentType())) {
97-
Utils.captureResponseBody(span, streamContextStore, writerContextStore, httpResponse);
106+
Utils.captureResponseBody(
107+
span, httpResponse, responseContextStore, streamContextStore, writerContextStore);
98108
}
99109

100110
if (agentConfig.getDataCapture().getHttpHeaders().getResponse().getValue()) {
@@ -111,7 +121,8 @@ private void captureResponseDataAndClearRequestBuffer(
111121
// remove request body buffers from context stores, otherwise they might get reused
112122
if (agentConfig.getDataCapture().getHttpBody().getRequest().getValue()
113123
&& ContentTypeUtils.shouldCapture(httpRequest.getContentType())) {
114-
Utils.resetRequestBodyBuffers(inputStreamContext, readerContext, httpRequest);
124+
Utils.resetRequestBodyBuffers(
125+
httpRequest, requestContextStore, inputStreamContextStore, readerContextStore);
115126
}
116127
}
117128
}

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/Servlet31NoWrappingInstrumentation.java

+29-15
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,10 @@
2828
import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
2929
import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
3030
import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
31+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.RequestStreamReaderHolder;
32+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ResponseStreamWriterHolder;
3133
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
3234
import java.io.BufferedReader;
33-
import java.io.IOException;
3435
import java.io.PrintWriter;
3536
import java.util.Enumeration;
3637
import java.util.HashMap;
@@ -102,8 +103,8 @@ public static boolean start(
102103
&& ContentTypeUtils.shouldCapture(contentType)) {
103104
// The HttpServletRequest instrumentation uses this to
104105
// enable the instrumentation
105-
InstrumentationContext.get(HttpServletRequest.class, Span.class)
106-
.put(httpRequest, currentSpan);
106+
InstrumentationContext.get(HttpServletRequest.class, RequestStreamReaderHolder.class)
107+
.put(httpRequest, new RequestStreamReaderHolder(currentSpan));
107108
}
108109

109110
Utils.addSessionId(currentSpan, httpRequest);
@@ -135,8 +136,7 @@ public static boolean start(
135136
public static void exit(
136137
@Advice.Argument(0) ServletRequest request,
137138
@Advice.Argument(1) ServletResponse response,
138-
@Advice.Local("currentSpan") Span currentSpan)
139-
throws IOException {
139+
@Advice.Local("currentSpan") Span currentSpan) {
140140
int callDepth =
141141
CallDepthThreadLocalMap.decrementCallDepth(Servlet31InstrumentationName.class);
142142
if (callDepth > 0) {
@@ -152,15 +152,21 @@ public static void exit(
152152
HttpServletRequest httpRequest = (HttpServletRequest) request;
153153
AgentConfig agentConfig = HypertraceConfig.get();
154154

155-
ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> outputStreamContext =
155+
ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> outputStreamContextStore =
156156
InstrumentationContext.get(ServletOutputStream.class, BoundedByteArrayOutputStream.class);
157-
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContext =
157+
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore =
158158
InstrumentationContext.get(PrintWriter.class, BoundedCharArrayWriter.class);
159159

160+
// response context to capture body and clear the context
161+
ContextStore<HttpServletResponse, ResponseStreamWriterHolder> responseContextStore =
162+
InstrumentationContext.get(HttpServletResponse.class, ResponseStreamWriterHolder.class);
163+
160164
// request context to clear body buffer
161-
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext =
165+
ContextStore<HttpServletRequest, RequestStreamReaderHolder> requestContextStore =
166+
InstrumentationContext.get(HttpServletRequest.class, RequestStreamReaderHolder.class);
167+
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContextStore =
162168
InstrumentationContext.get(ServletInputStream.class, ByteBufferSpanPair.class);
163-
ContextStore<BufferedReader, CharBufferSpanPair> readerContext =
169+
ContextStore<BufferedReader, CharBufferSpanPair> readerContextStore =
164170
InstrumentationContext.get(BufferedReader.class, CharBufferSpanPair.class);
165171

166172
AtomicBoolean responseHandled = new AtomicBoolean(false);
@@ -172,10 +178,12 @@ public static void exit(
172178
new BodyCaptureAsyncListener(
173179
responseHandled,
174180
currentSpan,
175-
outputStreamContext,
176-
writerContext,
177-
inputStreamContext,
178-
readerContext));
181+
responseContextStore,
182+
outputStreamContextStore,
183+
writerContextStore,
184+
requestContextStore,
185+
inputStreamContextStore,
186+
readerContextStore));
179187
} catch (IllegalStateException e) {
180188
// org.eclipse.jetty.server.Request may throw an exception here if request became
181189
// finished after check above. We just ignore that exception and move on.
@@ -194,13 +202,19 @@ public static void exit(
194202
// capture response body
195203
if (agentConfig.getDataCapture().getHttpBody().getResponse().getValue()
196204
&& ContentTypeUtils.shouldCapture(httpResponse.getContentType())) {
197-
Utils.captureResponseBody(currentSpan, outputStreamContext, writerContext, httpResponse);
205+
Utils.captureResponseBody(
206+
currentSpan,
207+
httpResponse,
208+
responseContextStore,
209+
outputStreamContextStore,
210+
writerContextStore);
198211
}
199212

200213
// remove request body buffers from context stores, otherwise they might get reused
201214
if (agentConfig.getDataCapture().getHttpBody().getRequest().getValue()
202215
&& ContentTypeUtils.shouldCapture(httpRequest.getContentType())) {
203-
Utils.resetRequestBodyBuffers(inputStreamContext, readerContext, httpRequest);
216+
Utils.resetRequestBodyBuffers(
217+
httpRequest, requestContextStore, inputStreamContextStore, readerContextStore);
204218
}
205219
}
206220
}

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/Servlet31NoWrappingInstrumentationModule.java

+5-2
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@
1919
import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed;
2020

2121
import com.google.auto.service.AutoService;
22-
import io.opentelemetry.api.trace.Span;
22+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.RequestStreamReaderHolder;
2323
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.ServletInputStreamInstrumentation;
2424
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.ServletRequestInstrumentation;
25+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ResponseStreamWriterHolder;
2526
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ServletOutputStreamInstrumentation;
2627
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ServletResponseInstrumentation;
2728
import io.opentelemetry.javaagent.tooling.InstrumentationModule;
@@ -67,11 +68,13 @@ public List<TypeInstrumentation> typeInstrumentations() {
6768
protected Map<String, String> contextStore() {
6869
Map<String, String> context = new HashMap<>();
6970
// capture request body
70-
context.put("javax.servlet.http.HttpServletRequest", Span.class.getName());
71+
context.put("javax.servlet.http.HttpServletRequest", RequestStreamReaderHolder.class.getName());
7172
context.put("javax.servlet.ServletInputStream", ByteBufferSpanPair.class.getName());
7273
context.put("java.io.BufferedReader", CharBufferSpanPair.class.getName());
7374

7475
// capture response body
76+
context.put(
77+
"javax.servlet.http.HttpServletResponse", ResponseStreamWriterHolder.class.getName());
7578
context.put("javax.servlet.ServletOutputStream", BoundedByteArrayOutputStream.class.getName());
7679
context.put("java.io.PrintWriter", BoundedCharArrayWriter.class.getName());
7780
return context;

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/Utils.java

+48-37
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,11 @@
1818

1919
import io.opentelemetry.api.trace.Span;
2020
import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
21+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request.RequestStreamReaderHolder;
22+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.response.ResponseStreamWriterHolder;
2123
import java.io.BufferedReader;
22-
import java.io.IOException;
2324
import java.io.PrintWriter;
25+
import java.io.UnsupportedEncodingException;
2426
import javax.servlet.ServletInputStream;
2527
import javax.servlet.ServletOutputStream;
2628
import javax.servlet.http.HttpServletRequest;
@@ -47,52 +49,61 @@ public static void addSessionId(Span span, HttpServletRequest httpRequest) {
4749

4850
public static void captureResponseBody(
4951
Span span,
52+
HttpServletResponse httpServletResponse,
53+
ContextStore<HttpServletResponse, ResponseStreamWriterHolder> responseContextStore,
5054
ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> streamContextStore,
51-
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore,
52-
HttpServletResponse httpResponse) {
55+
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore) {
5356

54-
try {
55-
ServletOutputStream outputStream = httpResponse.getOutputStream();
56-
BoundedByteArrayOutputStream buffer = streamContextStore.get(outputStream);
57+
ResponseStreamWriterHolder responseStreamWriterHolder =
58+
responseContextStore.get(httpServletResponse);
59+
if (responseStreamWriterHolder == null) {
60+
return;
61+
}
62+
responseContextStore.put(httpServletResponse, null);
63+
64+
if (responseStreamWriterHolder.getServletOutputStream() != null) {
65+
ServletOutputStream servletOutputStream = responseStreamWriterHolder.getServletOutputStream();
66+
BoundedByteArrayOutputStream buffer = streamContextStore.get(servletOutputStream);
5767
if (buffer != null) {
58-
span.setAttribute(
59-
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, buffer.toStringWithSuppliedCharset());
60-
streamContextStore.put(outputStream, null);
61-
}
62-
} catch (IllegalStateException | IOException exOutStream) {
63-
// getWriter was called
64-
try {
65-
PrintWriter writer = httpResponse.getWriter();
66-
BoundedCharArrayWriter buffer = writerContextStore.get(writer);
67-
if (buffer != null) {
68-
span.setAttribute(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, buffer.toString());
69-
writerContextStore.put(writer, null);
68+
try {
69+
span.setAttribute(
70+
HypertraceSemanticAttributes.HTTP_RESPONSE_BODY,
71+
buffer.toStringWithSuppliedCharset());
72+
} catch (UnsupportedEncodingException e) {
73+
// should not happen
7074
}
71-
} catch (IllegalStateException | IOException exPrintWriter) {
75+
streamContextStore.put(servletOutputStream, null);
76+
}
77+
}
78+
79+
if (responseStreamWriterHolder.getPrintWriter() != null) {
80+
PrintWriter printWriter = responseStreamWriterHolder.getPrintWriter();
81+
BoundedCharArrayWriter buffer = writerContextStore.get(printWriter);
82+
if (buffer != null) {
83+
span.setAttribute(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, buffer.toString());
84+
writerContextStore.put(printWriter, null);
7285
}
7386
}
7487
}
7588

7689
public static void resetRequestBodyBuffers(
90+
HttpServletRequest httpServletRequest,
91+
ContextStore<HttpServletRequest, RequestStreamReaderHolder> requestContextStore,
7792
ContextStore<ServletInputStream, ByteBufferSpanPair> streamContextStore,
78-
ContextStore<BufferedReader, CharBufferSpanPair> printContextStore,
79-
HttpServletRequest httpRequest) {
80-
try {
81-
ServletInputStream inputStream = httpRequest.getInputStream();
82-
ByteBufferSpanPair bufferSpanPair = streamContextStore.get(inputStream);
83-
if (bufferSpanPair != null) {
84-
streamContextStore.put(inputStream, null);
85-
}
86-
} catch (IllegalStateException | IOException exOutStream) {
87-
// getWriter was called
88-
try {
89-
BufferedReader reader = httpRequest.getReader();
90-
CharBufferSpanPair bufferSpanPair = printContextStore.get(reader);
91-
if (bufferSpanPair != null) {
92-
printContextStore.put(reader, null);
93-
}
94-
} catch (IllegalStateException | IOException exPrintWriter) {
95-
}
93+
ContextStore<BufferedReader, CharBufferSpanPair> bufferedReaderContextStore) {
94+
95+
RequestStreamReaderHolder requestStreamReaderHolder =
96+
requestContextStore.get(httpServletRequest);
97+
if (requestContextStore == null) {
98+
return;
99+
}
100+
requestContextStore.put(httpServletRequest, null);
101+
102+
if (requestStreamReaderHolder.getServletInputStream() != null) {
103+
streamContextStore.put(requestStreamReaderHolder.getServletInputStream(), null);
104+
}
105+
if (requestStreamReaderHolder.getBufferedReader() != null) {
106+
bufferedReaderContextStore.put(requestStreamReaderHolder.getBufferedReader(), null);
96107
}
97108
}
98109
}

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/request/RequestStreamReaderHolder.java

+52
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
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.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.request;
18+
19+
import io.opentelemetry.api.trace.Span;
20+
import java.io.BufferedReader;
21+
import javax.servlet.ServletInputStream;
22+
23+
public class RequestStreamReaderHolder {
24+
25+
private final Span span;
26+
private ServletInputStream servletInputStream;
27+
private BufferedReader bufferedReader;
28+
29+
public RequestStreamReaderHolder(Span span) {
30+
this.span = span;
31+
}
32+
33+
public Span getSpan() {
34+
return span;
35+
}
36+
37+
public ServletInputStream getServletInputStream() {
38+
return servletInputStream;
39+
}
40+
41+
public void setServletInputStream(ServletInputStream servletInputStream) {
42+
this.servletInputStream = servletInputStream;
43+
}
44+
45+
public BufferedReader getBufferedReader() {
46+
return bufferedReader;
47+
}
48+
49+
public void setBufferedReader(BufferedReader bufferedReader) {
50+
this.bufferedReader = bufferedReader;
51+
}
52+
}

0 commit comments

Comments
 (0)