Skip to content

Log HTTP method in logging filters and revise log message format#23567

Closed
schnapster wants to merge 4 commits into
spring-projects:masterfrom
schnapster:log-http-method
Closed

Log HTTP method in logging filters and revise log message format#23567
schnapster wants to merge 4 commits into
spring-projects:masterfrom
schnapster:log-http-method

Conversation

@schnapster

@schnapster schnapster commented Sep 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ahoy,

I was surprised to neither find the http method in the log messages of the AbstractRequestLoggingFilter nor the option to turn them on.

This looks like a fairly uncontroversial, fully optional and backwards compatible change to me so I did not open an issue up front. I'm happy to open one if more discussion is required.

Two things to note for the reviewer:

- [ ] I copy-pasted the method javadocs, and I'm not 100% sure if anything else needs to be done to get

	 * <p>Should be configured using an {@code <init-param>} for parameter name
	 * "includeHttpMethod" in the filter definition in {@code web.xml}.

to work. I did some CTRL-Fing for the other values and nothing interesting came up.

- [ ] I did not add a @since 5.2 annotation as I'm not sure which version this will hit. Just amend it in :)

Cheers,
Napster

Btw, setting up the local build and running tests was a breeze, great docs!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 2, 2019
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Sep 12, 2019
@sbrannen

sbrannen commented Sep 13, 2019

Copy link
Copy Markdown
Member

After a quick discussion with @rstoyanchev, we have decided that we should simply always include the HTTP method in those logs messages. In other words, there's no need to make it optional.

Can you please rework this PR accordingly?

@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 13, 2019
@sbrannen sbrannen added this to the 5.2 GA milestone Sep 13, 2019
@sbrannen

sbrannen commented Sep 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Team Decision:

After some further internal brainstorming, we have decided on the following.

  • Always include the HTTP method.
  • Change the format of the generated log message to the following, keeping the existing optional behavior in tact for the query string, client info, headers, and payload.

Format: <prefix><method> <uri + query string>, <client>, <session>, <user>, <headers>, <payload><suffix>

Example: Before request [GET /foo?x=2, client=..., session=..., user=..., headers=..., payload=...]

@napstr, please let us know if you are willing to rework this PR along those lines.

Thanks

@sbrannen sbrannen changed the title Add option to log HTTP method Log HTTP method in logging filters and revise log message format Sep 13, 2019
@schnapster

schnapster commented Sep 13, 2019

Copy link
Copy Markdown
Contributor Author

Will do, I'll aim to get this done over the weekend :)

Just to clarify, did you also decide to change the existing ;s to , s, @sbrannen or that a typo in the format/example?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 13, 2019
@sbrannen

sbrannen commented Sep 13, 2019

Copy link
Copy Markdown
Member

The Spring Team would also like to point out that the logRecord() method in the DispatcherServlet is capable of logging very detailed information at DEBUG and TRACE levels.

This detailed logging in the DispatcherServlet is available since Spring Framework 5.0 and may serve as an alternative to AbstractRequestLoggingFilter and its subclasses (e.g., CommonsRequestLoggingFilter, ServletContextRequestLoggingFilter, and custom implementations).

Consult the Logging section of the Spring Web MVC documentation for further details.

@sbrannen

sbrannen commented Sep 13, 2019

Copy link
Copy Markdown
Member

Will do, I'll aim to get this done over the weekend :)

Great!

Just to clarify, did you also decide to change the existing ;s to , s, @sbrannen or that a typo in the format/example?

Yes, we decided to change the delimiter from ; to , (with the comma) in order to make it more human readable and to align with the format in org.springframework.web.servlet.DispatcherServlet.logRequest(HttpServletRequest).

@schnapster schnapster force-pushed the log-http-method branch 2 times, most recently from 0995005 to 3fdca2f Compare September 14, 2019 00:23
@schnapster

schnapster commented Sep 14, 2019

Copy link
Copy Markdown
Contributor Author

How does this look?

sbrannen
sbrannen requested changes Sep 14, 2019
View reviewed changes

@sbrannen sbrannen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes look good.

I've requested simplifications to the tests. So please make those and perhaps add an additional test that's more inclusive -- for example, one that contains everything like the previously posted example: Before request [GET /foo?x=2, client=..., session=..., user=..., headers=..., payload=...].

Thanks

Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java
Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java Outdated
Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java Outdated
Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java Outdated
Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java Outdated
Comment thread spring-web/src/test/java/org/springframework/web/filter/RequestLoggingFilterTests.java Outdated
@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 14, 2019
@sbrannen sbrannen self-assigned this Sep 14, 2019
@schnapster

schnapster commented Sep 14, 2019

Copy link
Copy Markdown
Contributor Author

@sbrannen thank you for the hints on how to improve usage of the available AssertJ api. I believe I applied all your suggestions now - let me know if anything is left.

@spring-projects-issues spring-projects-issues added the status: feedback-provided Feedback has been provided label Sep 14, 2019
@spring-projects-issues spring-projects-issues removed the status: waiting-for-feedback We need additional information before we can continue label Sep 14, 2019
@sbrannen

sbrannen commented Sep 14, 2019

Copy link
Copy Markdown
Member

@sbrannen thank you for the hints on how to improve usage of the available AssertJ api.

Glad to be of service.

I believe I applied all your suggestions now - let me know if anything is left.

I think that looks good now. I've accepted the changes and will merge the PR in the coming days.

Enjoy your weekend!

@sbrannen sbrannen closed this in 33cee06 Sep 16, 2019
@sbrannen

sbrannen commented Sep 16, 2019

Copy link
Copy Markdown
Member

This has been merged into master in 33cee06, and the tests have been further polished in 52128fe.

Thanks for the PR

@schnapster schnapster deleted the log-http-method branch September 16, 2019 15:01
@rstoyanchev rstoyanchev mentioned this pull request Oct 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbrannen sbrannen sbrannen approved these changes

Assignees

@sbrannen sbrannen

@schnapster schnapster

Labels

in: web Issues in web modules (web, webmvc, webflux, websocket) status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Projects

None yet

Milestone

5.2 GA

Development

Successfully merging this pull request may close these issues.

3 participants

@schnapster @sbrannen @spring-projects-issues