Add ability to extend HttpTraceWebFilter (e.g. to include req/resp body)

[gberche-orange]

Expected behavior

As a springboot user

• in order to extend the behavior of actuator httptrace endpoint, such as to include request/response body as discussed
  Unable to add request/response body to httptrace (actuator) 2.0.1-RELEASE #12953 (comment)
• I need extension points in the HttpTraceWebFilter to be able to cleanly inject new behavior without duplicating most of the webfilter in custom code.

Current behavior

While a custom implementation of HttpTraceWebFilter can be injected thanks to

spring-boot/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/trace/http/HttpTraceAutoConfiguration.java

Lines 64 to 73 in a669e1c

	@Configuration(proxyBeanMethods = false)
	@ConditionalOnWebApplication(type = Type.REACTIVE)
	static class ReactiveTraceFilterConfiguration {
	@Bean
	@ConditionalOnMissingBean
	HttpTraceWebFilter httpTraceWebFilter(HttpTraceRepository repository, HttpExchangeTracer tracer,
	HttpTraceProperties traceProperties) {
	return new HttpTraceWebFilter(repository, tracer, traceProperties.getInclude());
	}

it requires the following workarounds:

• the custom HttpTraceWebFilter class needs to extend HttpTraceWebFilter

• the ServerWebExchangeTraceableRequest and TraceableServerHttpResponse objects can't be injected as beans and are instead hard coded

• the ServerWebExchangeTraceableRequest and TraceableServerHttpResponse classes are package protected preventing them from being subclassed to enrich their behavior.

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/trace/reactive/ServerWebExchangeTraceableRequest.java

Line 35 in a669e1c

class ServerWebExchangeTraceableRequest implements TraceableRequest {

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/trace/reactive/TraceableServerHttpResponse.java

Line 32 in a669e1c

class TraceableServerHttpResponse implements TraceableResponse {

• finally, the request is inserted into the tracer synchronously when the filter is invoked. This prevents other filters (such as spring cloud gateway filters capturing request body) for executing before. There is a need to insert the request into the tracer as a response precommit step (along with the response).

spring-boot/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/trace/reactive/HttpTraceWebFilter.java

Lines 85 to 96 in a669e1c

	private Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain, Principal principal,
	WebSession session) {
	ServerWebExchangeTraceableRequest request = new ServerWebExchangeTraceableRequest(exchange);
	HttpTrace trace = this.tracer.receivedRequest(request);
	exchange.getResponse().beforeCommit(() -> {
	TraceableServerHttpResponse response = new TraceableServerHttpResponse(exchange.getResponse());
	this.tracer.sendingResponse(trace, response, () -> principal, () -> getStartedSessionId(session));
	this.repository.add(trace);
	return Mono.empty();
	});
	return chain.filter(exchange);
	}

Here is the full resulting duplicated webfilter along with associated spring cloud gateway configuration https://gist.github.com/gberche-orange/06c26477a313df9d19d20a4e115f079f that injects request and response bodies as respectively request_body and response_body http headers

[wilkinsona]

The scope of HTTP tracing was deliberately narrowed and locked down in 2.0 so that both the repository that stores the traces and the endpoint that returns them would have predictable data to deal with. Opening up the API as you have suggested will remove the predictability which is something that I don't think we want to do. I'm also not keen on opening things up to record request and response bodies as doing so is generally not recommended as it requires buffering the entire body in memory. This is only my opinion so I'll mark this one for team attention to see what the rest of the team thinks.

As we mention in the documentation, if your needs have outgrown the capabilities of the built-in HTTP tracing they may be better served by using something like Zipkin or Spring Cloud Sleuth.

[gberche-orange]

@wilkinsona thanks for your response.

As we mention in the documentation, if your needs have outgrown the capabilities of the built-in HTTP tracing they may be better served by using something like Zipkin or Spring Cloud Sleuth.

yes, I had noticed the mention and have investigated this approach, however the requirement to trace http request/response bodies is not supported either brave and zipkin and likely to be even more complex

See https://gitter.im/openzipkin/zipkin?at=5ca0156693fb4a7dc2a9e791 march 2019

@kirenjolly right now we don't expose getting the http body generically.
We probably should add a comment to that file as to why. Not only does this likely result in things too big to put into a span, but also it can cause problems in your production requests. For example, not all http bodies are replayable.. ex if it is a stream and we consume it, the app couldn't.
you can ask here about "blob service" which is what @jcchavezs was referring to.

Brave seems to abstract out the http client and server and hide away the ServerWebExchange, see https://github.com/openzipkin/brave/tree/master/instrumentation/http#span-data-policy

 By default, the following are added to both http client and server spans:
     Span.name is the http method in lowercase: ex "get" or a route described below
     Tags:
         "http.method", eg "GET"
         "http.path", which does not include query parameters.
         "http.status_code" when the status is not success.
         "error", when there is an exception or status is >=400
     Remote IP and port information

Logging req/resp body in brave would possibly require

• a custom netty HttpTracing impl
  * See base impl at https://github.com/openzipkin/brave/blob/3a8bb062f68d6729f80c38969e87c32ce42a7681/instrumentation/netty-codec-http/src/main/java/brave/netty/http/TracingHttpServerHandler.java#L35
  * See https://github.com/netty/netty/blob/c061bd17986785426552df659a30f4ada491350c/handler/src/main/java/io/netty/handler/logging/LoggingHandler.java#L40 about built-in netty logger
  * No evidence of HttpTracing http impl in sleuth source code that would add request trace

the repository that stores the traces and the endpoint that returns them would have predictable data to deal with. Opening up the API as you have suggested will remove the predictability which is something that I don't think we want to do.

I do agree that blindly adding request and response body to the actuator httptracing regardless of served traffic will impact predictability and isn't a desired goal for a general purpose usage.

My specific use-case is a reverse proxy based on spring-cloud-gateway, proxifying low traffic and small API requests/responses where inspection of proxied traffic is necessary. The side effect on memory (denial of service or predictability) is also mitigated by abbreviating the recorded bodies and removing them from the exchange attributes (as to favor GC).

Maybe there is room to support users in http tracing extensions for such specific use-cases.

[wilkinsona]

We've discussed this today and the team agreed that we don't want to increase the surface area of the public API to support this use case. Our recommendation is that you maintain your own copy of the current code, modified to suit your specific needs. Thanks anyway for the suggestion.

[gberche-orange]

thanks for your response and for having considering the suggestion.