Provide customized access log

[immuthex]

No description provided.

[violetagg]

Related issues #436 and #476

[violetagg]

@limaoning @simonbasle What do you think if the access log functionality is moved to its own package (e.g. logging)?

@limaoning Can you add javadoc to the public API? I like the idea, once we agree on the package and we have a javadoc, I'll make a detailed review.

[simonbasle]

@limaoning @simonbasle What do you think if the access log functionality is moved to its own package (e.g. logging)?

Yeah that makes sense to me if that possible 👍

[immuthex]

@violetagg The code has been submitted, please take a look.

[codecov-io]

Codecov Report

Merging #1357 into master will increase coverage by 1.06%.
The diff coverage is 87.83%.

@@             Coverage Diff              @@
##             master    #1357      +/-   ##
============================================
+ Coverage     69.45%   70.52%   +1.06%     
- Complexity     1768     1826      +58     
============================================
  Files           139      144       +5     
  Lines          8998     9071      +73     
  Branches       1222     1232      +10     
============================================
+ Hits           6250     6397     +147     
+ Misses         2098     2013      -85     
- Partials        650      661      +11     

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/reactor/netty/http/server/HttpServer.java	64.33% <0.00%> (-1.69%)	31.00 <0.00> (ø)
...etty/http/server/logging/BaseAccessLogHandler.java	72.72% <72.72%> (ø)	5.00 <5.00> (?)
...va/reactor/netty/http/server/HttpServerConfig.java	81.42% <82.35%> (+0.33%)	35.00 <0.00> (ø)
.../netty/http/server/logging/AccessLogHandlerH2.java	87.50% <83.33%> (ø)	7.00 <1.00> (?)
.../netty/http/server/logging/AccessLogHandlerH1.java	81.08% <86.66%> (ø)	9.00 <1.00> (?)
...a/reactor/netty/http/server/logging/AccessLog.java	90.00% <90.00%> (ø)	4.00 <4.00> (?)
...ty/http/server/logging/AccessLogArgProviderH2.java	94.11% <94.11%> (ø)	7.00 <7.00> (?)
...ty/http/server/logging/AccessLogArgProviderH1.java	94.73% <94.73%> (ø)	8.00 <8.00> (?)
...p/server/logging/AbstractAccessLogArgProvider.java	97.29% <97.29%> (ø)	19.00 <19.00> (?)
...y/http/server/logging/AccessLogHandlerFactory.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83f66a8...20e1896. Read the comment docs.

[simonbasle]

to maintain consistency, I'd rename this class to AccessLogHandlerH1.

@violetagg @limaoning I'm also wondering about the public status of this and the H2 implementations. maybe moving everything in logging package wasn't such a good idea 😖

I wonder if it wouldn't be better to put the concrete classes back into the base http.server package, as package-private, and only keep the interfaces and user-facing classes here in http.server.logging. ie classes that make sense for a user to implement/customize/inherit from

[violetagg]

to maintain consistency, I'd rename this class to AccessLogHandlerH1.

Agree with that

And let's move to the http.server package ONLY the handlers BaseAccessLogHandler, AccessLogHandlerH1, AccessLogHandlerH2

[immuthex]

But AccessLogArgProviderH1, AccessLogArgProviderH2 will be public.

[violetagg]

if we move them to http.server they can be package private - that's the idea

[immuthex]

You are saying move handlers to http.server, but AccessLogArgProviderH1, AccessLogArgProviderH2 are not, and they will be public. Otherwise, move AccessLogArgProviderH1, AccessLogArgProviderH2 to http.server too?

[violetagg]

@simonbasle Here I think we either move everything or keep the handlers public as @limaoning noted in the comment above

[violetagg]

Let me think a bit here

[violetagg]

some rough idea

public class AccessLogHandlerFactory {

	ChannelHandler newAccessLogHandler(@Nullable Function<AccessLogArgProvider, AccessLog> accessLog, boolean isHttp2) {
		return isHttp2 ? new AccessLogHandlerH2(accessLog) : new AccessLogHandlerH1(accessLog);
	}
}

wdyt?

[violetagg]

or may be even something like this https://github.com/reactor/reactor-netty/blob/master/reactor-netty-core/src/main/java/reactor/netty/transport/logging/AdvancedByteBufFormat.java

[immuthex]

Should be OK.

[violetagg]

@simonbasle PTAL

[violetagg]

@limaoning I squashed and committed the changes 9e44b6d. Thanks for that contribution!

[IvanSmurygin]

Hello! Do you have some readme regarding changes for access log?

[violetagg]

@IvanSmurygin Currently there is no reference documentation for this particular change. Are you interested in providing a PR with such?

[nikban95]

There is still no way to log response headers in access log even after the above change. Are we planning to add it in future??

[violetagg]

@nikban95 By default Reactor Netty provides Common Log Format. If you need another format you can easily provide it by yourself. Here is a documentation:
https://projectreactor.io/docs/netty/release/reference/index.html#_http_access_log

Please ask you questions on Gitter or stackoverflow.com

[immuthex]

@violetagg What he means is that there is no way to get response header, only the method to get request header.

[nikban95]

@violetagg As @limaoning has pointed out. Currently we can only log request headers only but the response headers can't be logged. So was asking about the same.

[violetagg]

@nikban95 @limaoning mmmm correct. Can you create an issue to expose the response headers via AccessLogArgProvider. Are you interested in providing a PR?

[nikban95]

Sure @violetagg. Created an issue: #1505. Will check if i can provide the pr for the same.

[violetagg]

Sure @violetagg. Created an issue: #1505. Will check if i can provide the pr for the same.

great!