Skip to content

Commit

Permalink
[pinpoint-apm#10355] Fix servlet response http-status-recorder
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaehong-kim
jaehong-kim committed Sep 20, 2023
1 parent 5eb8d5f commit 3b7c65d
Show file tree
Hide file tree
Showing 13 changed files with 19 additions and 55 deletions.
6 changes: 4 additions & 2 deletions ...re/src/main/java/com/navercorp/pinpoint/bootstrap/plugin/http/HttpStatusCodeRecorder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import com.navercorp.pinpoint.bootstrap.logging.PLogger;
import com.navercorp.pinpoint.bootstrap.logging.PLoggerFactory;

import java.util.Objects;

/**
* @author jaehong.kim
*/
Expand All @@ -31,7 +33,7 @@ public class HttpStatusCodeRecorder {
private final HttpStatusCodeErrors errors;

public HttpStatusCodeRecorder(final HttpStatusCodeErrors errors) {
this.errors = errors;
this.errors = Objects.requireNonNull(errors, "errors");
}

public void record(final SpanRecorder spanRecorder, final int statusCode) {
Expand All @@ -40,7 +42,7 @@ public void record(final SpanRecorder spanRecorder, final int statusCode) {
return;
}

if (!this.errors.isHttpStatusCode(statusCode)) {
if (Boolean.FALSE == this.errors.isHttpStatusCode(statusCode)) {
if (isDebug) {
logger.debug("Out of range HTTP status code. statusCode={}", statusCode);
}
Expand Down
11 changes: 2 additions & 9 deletions ...src/main/java/com/navercorp/pinpoint/bootstrap/plugin/request/ServletRequestListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ public class ServletRequestListener<REQ> {

private final ProxyRequestRecorder<REQ> proxyRequestRecorder;

private final boolean recordStatusCode;

public ServletRequestListener(final ServiceType serviceType,
final TraceContext traceContext,
final RequestAdaptor<REQ> requestAdaptor,
Expand All @@ -65,8 +63,7 @@ public ServletRequestListener(final ServiceType serviceType,
final ParameterRecorder<REQ> parameterRecorder,
final ProxyRequestRecorder<REQ> proxyRequestRecorder,
final ServerRequestRecorder<REQ> serverRequestRecorder,
final HttpStatusCodeRecorder httpStatusCodeRecorder,
final boolean recordStatusCode) {
final HttpStatusCodeRecorder httpStatusCodeRecorder) {
this.serviceType = Objects.requireNonNull(serviceType, "serviceType");
this.traceContext = Objects.requireNonNull(traceContext, "traceContext");
this.requestAdaptor = Objects.requireNonNull(requestAdaptor, "requestAdaptor");
Expand All @@ -76,7 +73,6 @@ public ServletRequestListener(final ServiceType serviceType,
this.parameterRecorder = Objects.requireNonNull(parameterRecorder, "parameterRecorder");
this.serverRequestRecorder = Objects.requireNonNull(serverRequestRecorder, "serverRequestRecorder");
this.httpStatusCodeRecorder = Objects.requireNonNull(httpStatusCodeRecorder, "httpStatusCodeRecorder");
this.recordStatusCode = recordStatusCode;
this.traceContext.cacheApi(SERVLET_SYNC_METHOD_DESCRIPTOR);
}

Expand Down Expand Up @@ -141,11 +137,8 @@ public void destroyed(REQ request, final Throwable throwable, final int statusCo
return;
}

if (this.recordStatusCode) {
this.httpStatusCodeRecorder.record(trace.getSpanRecorder(), statusCode);
}

try {
this.httpStatusCodeRecorder.record(trace.getSpanRecorder(), statusCode);
final SpanEventRecorder recorder = trace.currentSpanEventRecorder();
if (trace.canSampled()) {
recorder.recordException(throwable);
Expand Down
7 changes: 1 addition & 6 deletions ...n/java/com/navercorp/pinpoint/bootstrap/plugin/request/ServletRequestListenerBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public class ServletRequestListenerBuilder<REQ> {

private List<String> recordRequestHeaders;
private List<String> recordRequestCookies;
private boolean recordStatusCode = true;

public ServletRequestListenerBuilder(final ServiceType serviceType,
final TraceContext traceContext,
Expand Down Expand Up @@ -97,10 +96,6 @@ public void setServerCookieRecorder(List<String> recordRequestCookies) {
this.recordRequestCookies = recordRequestCookies;
}

public void setRecordStatusCode(boolean recordStatusCode) {
this.recordStatusCode = recordStatusCode;
}

private <T> Filter<T> newExcludeUrlFilter(Filter<T> excludeUrlFilter) {
if (excludeUrlFilter == null) {
return new SkipFilter<>();
Expand Down Expand Up @@ -141,7 +136,7 @@ public ServletRequestListener<REQ> build() {


return new ServletRequestListener<>(serviceType, traceContext, requestAdaptor, requestTraceReader,
excludeUrlFilter, parameterRecorder, proxyRequestRecorder, serverRequestRecorder, httpStatusCodeRecorder, recordStatusCode);
excludeUrlFilter, parameterRecorder, proxyRequestRecorder, serverRequestRecorder, httpStatusCodeRecorder);
}


Expand Down
9 changes: 2 additions & 7 deletions ...c/main/java/com/navercorp/pinpoint/bootstrap/plugin/response/ServletResponseListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.navercorp.pinpoint.bootstrap.context.TraceContext;
import com.navercorp.pinpoint.bootstrap.logging.PLogger;
import com.navercorp.pinpoint.bootstrap.logging.PLoggerFactory;
import com.navercorp.pinpoint.bootstrap.plugin.http.HttpStatusCodeRecorder;
import com.navercorp.pinpoint.common.trace.ServiceType;

import java.util.Objects;
Expand All @@ -37,14 +36,11 @@ public class ServletResponseListener<RESP> {

private final TraceContext traceContext;
private final ServerResponseHeaderRecorder<RESP> serverResponseHeaderRecorder;
private final HttpStatusCodeRecorder httpStatusCodeRecorder;

public ServletResponseListener(final TraceContext traceContext,
final ServerResponseHeaderRecorder<RESP> serverResponseHeaderRecorder,
final HttpStatusCodeRecorder httpStatusCodeRecorder) {
final ServerResponseHeaderRecorder<RESP> serverResponseHeaderRecorder) {
this.traceContext = Objects.requireNonNull(traceContext, "traceContext");
this.serverResponseHeaderRecorder = Objects.requireNonNull(serverResponseHeaderRecorder, "serverResponseHeaderRecorder");
this.httpStatusCodeRecorder = Objects.requireNonNull(httpStatusCodeRecorder, "statusCodeRecorder");
}


Expand Down Expand Up @@ -72,9 +68,8 @@ public void destroyed(RESP response, final Throwable throwable, final int status
return;
}

final SpanRecorder spanRecorder = trace.getSpanRecorder();
this.httpStatusCodeRecorder.record(spanRecorder, statusCode);
if (trace.canSampled()) {
final SpanRecorder spanRecorder = trace.getSpanRecorder();
this.serverResponseHeaderRecorder.recordHeader(spanRecorder, response);
}
}
Expand Down
26 changes: 2 additions & 24 deletions ...java/com/navercorp/pinpoint/bootstrap/plugin/response/ServletResponseListenerBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,10 @@

package com.navercorp.pinpoint.bootstrap.plugin.response;

import com.navercorp.pinpoint.bootstrap.config.HttpStatusCodeErrors;
import com.navercorp.pinpoint.bootstrap.config.ProfilerConfig;
import com.navercorp.pinpoint.bootstrap.context.TraceContext;
import com.navercorp.pinpoint.bootstrap.plugin.http.HttpStatusCodeRecorder;
import com.navercorp.pinpoint.common.util.CollectionUtils;

import java.util.Collections;
import java.util.List;
import java.util.Objects;

Expand All @@ -35,36 +32,18 @@ public class ServletResponseListenerBuilder<RESP> {

private List<String> recordResponseHeaders;

private HttpStatusCodeErrors httpStatusCodeErrors;

public ServletResponseListenerBuilder(final TraceContext traceContext,
final ResponseAdaptor<RESP> responseAdaptor) {
this.traceContext = Objects.requireNonNull(traceContext, "traceContext");
this.responseAdaptor = Objects.requireNonNull(responseAdaptor, "responseAdaptor");

final ProfilerConfig profilerConfig = traceContext.getProfilerConfig();
final List<String> recordResponseHeaders = profilerConfig.readList(ServerResponseHeaderRecorder.CONFIG_KEY_RECORD_RESP_HEADERS);
setRecordResponseHeaders(recordResponseHeaders);
setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
}

public void setRecordResponseHeaders(List<String> recordResponseHeaders) {
this.recordResponseHeaders = recordResponseHeaders;
}

public void setHttpStatusCodeRecorder(final HttpStatusCodeErrors httpStatusCodeErrors) {
this.httpStatusCodeErrors = httpStatusCodeErrors;
}

public ServletResponseListener<RESP> build() {
HttpStatusCodeRecorder httpStatusCodeRecorder;
if (httpStatusCodeErrors == null) {
HttpStatusCodeErrors httpStatusCodeErrors = new HttpStatusCodeErrors(Collections.emptyList());
httpStatusCodeRecorder = new HttpStatusCodeRecorder(httpStatusCodeErrors);
} else {
httpStatusCodeRecorder = new HttpStatusCodeRecorder(httpStatusCodeErrors);
}
return new ServletResponseListener<>(traceContext, newServerResponseHeaderRecorder(), httpStatusCodeRecorder);
return new ServletResponseListener<>(traceContext, newServerResponseHeaderRecorder());
}

private ServerResponseHeaderRecorder<RESP> newServerResponseHeaderRecorder() {
Expand All @@ -73,5 +52,4 @@ private ServerResponseHeaderRecorder<RESP> newServerResponseHeaderRecorder() {
}
return new DefaultServerResponseHeaderRecorder<>(responseAdaptor, recordResponseHeaders);
}

}
}
8 changes: 8 additions & 0 deletions ...rc/test/java/com/navercorp/pinpoint/bootstrap/plugin/http/HttpStatusCodeRecorderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.navercorp.pinpoint.bootstrap.config.HttpStatusCodeErrors;
import com.navercorp.pinpoint.bootstrap.context.SpanRecorder;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.util.Arrays;
Expand Down Expand Up @@ -46,4 +47,11 @@ public void record() throws Exception {
recorder.record(spanRecorder, -1);
recorder.record(spanRecorder, 999);
}

@Test
public void errorsIsNull() throws Exception {
Assertions.assertThrows(NullPointerException.class, () -> {
HttpStatusCodeRecorder recorder = new HttpStatusCodeRecorder(null);
});
}
}
1 change: 0 additions & 1 deletion ...a/com/navercorp/pinpoint/plugin/jboss/interceptor/StandardHostValveInvokeInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ public StandardHostValveInvokeInterceptor(final TraceContext traceContext, final
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();

Expand Down
1 change: 0 additions & 1 deletion ...java/com/navercorp/pinpoint/plugin/jetty/interceptor/AbstractServerHandleInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ public AbstractServerHandleInterceptor(TraceContext traceContext, MethodDescript
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();

Expand Down
1 change: 0 additions & 1 deletion .../com/navercorp/pinpoint/plugin/reactor/netty/interceptor/HttpServerHandleInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public HttpServerHandleInterceptor(TraceContext traceContext,
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();
this.servletResponseListener = new ServletResponseListenerBuilder<>(traceContext, new HttpResponseAdaptor()).build();
Expand Down
1 change: 0 additions & 1 deletion .../com/navercorp/pinpoint/plugin/resin/interceptor/ServletInvocationServiceInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public ServletInvocationServiceInterceptor(TraceContext traceContext, MethodDesc
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();
this.servletResponseListener = new ServletResponseListenerBuilder<>(traceContext, new HttpServletResponseAdaptor()).build();
Expand Down
1 change: 0 additions & 1 deletion ...ercorp/pinpoint/plugin/tomcat/jakarta/interceptor/StandardHostValveInvokeInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public StandardHostValveInvokeInterceptor(TraceContext traceContext, MethodDescr
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();

Expand Down
1 change: 0 additions & 1 deletion ...avercorp/pinpoint/plugin/tomcat/javax/interceptor/StandardHostValveInvokeInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ public StandardHostValveInvokeInterceptor(TraceContext traceContext, MethodDescr
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();

Expand Down
1 change: 0 additions & 1 deletion ...vercorp/pinpoint/plugin/undertow/interceptor/ConnectorsExecuteRootHandlerInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public ConnectorsExecuteRootHandlerInterceptor(TraceContext traceContext, Method
reqBuilder.setHttpStatusCodeRecorder(profilerConfig.getHttpStatusCodeErrors());
reqBuilder.setServerHeaderRecorder(profilerConfig.readList(ServerHeaderRecorder.CONFIG_KEY_RECORD_REQ_HEADERS));
reqBuilder.setServerCookieRecorder(profilerConfig.readList(ServerCookieRecorder.CONFIG_KEY_RECORD_REQ_COOKIES));
reqBuilder.setRecordStatusCode(false);

this.servletRequestListener = reqBuilder.build();

Expand Down

0 comments on commit 3b7c65d

Please # to comment.