Use IHttpRequestFeature.Path in RequestLoggingMiddleware to log RequestPath

[jarronshih]

Why

See #232

RequestPath uses RawTarget which includes query data inside.

What

IHttpRequestFeature.Path could be a better choose.

To not break the existing behavior, we make a option RequestLoggingOptions.IncludeRawTargetPath with default value true.

Demo

Use sample.cs to test

# With options.IncludeRawTargetPath = false
2021-09-27 15:43:03.148 -07:00 [INF] Now listening on: https://localhost:5001
2021-09-27 15:43:03.181 -07:00 [INF] Now listening on: http://localhost:5000
2021-09-27 15:43:03.183 -07:00 [INF] Application started. Press Ctrl+C to shut down.
2021-09-27 15:43:03.185 -07:00 [INF] Hosting environment: Development
2021-09-27 15:43:03.187 -07:00 [INF] Content root path: C:\git\serilog-aspnetcore\samples\Sample
2021-09-27 15:43:03.942 -07:00 [INF] Hello, world!
2021-09-27 15:43:04.067 -07:00 [INF] HTTP GET / responded 200 in 183.3057 ms
2021-09-27 15:43:13.467 -07:00 [INF] HTTP GET /Index responded 404 in 0.4129 ms
2021-09-27 15:43:19.619 -07:00 [INF] HTTP GET /Index responded 404 in 0.0240 ms

# With options.IncludeRawTargetPath = true, which is default behavior
2021-09-27 15:43:35.651 -07:00 [INF] Now listening on: https://localhost:5001
2021-09-27 15:43:35.728 -07:00 [INF] Now listening on: http://localhost:5000
2021-09-27 15:43:35.745 -07:00 [INF] Application started. Press Ctrl+C to shut down.
2021-09-27 15:43:35.748 -07:00 [INF] Hosting environment: Development
2021-09-27 15:43:35.751 -07:00 [INF] Content root path: C:\git\serilog-aspnetcore\samples\Sample
2021-09-27 15:43:36.331 -07:00 [INF] Hello, world!
2021-09-27 15:43:36.496 -07:00 [INF] HTTP GET / responded 200 in 212.8998 ms
2021-09-27 15:43:42.311 -07:00 [INF] HTTP GET /Index?foo=bar responded 404 in 0.2009 ms

[sungam3r]

?

[nblumhardt]

This looks good! One last thought - RawTargetPath is a bit opaque - it will be hard to guess what it means without consulting the docs. What about if we try IncludeQueryInPath?

[jarronshih]

Thanks @nblumhardt ! I update the PR with the suggested name

[nblumhardt]

Thanks @jarronshih! Sorry to send one last round of requests 😅 but I've had some more time to think about this, and came to the following conclusions:

• The property should probably call it RequestPath, i.e. IncludeQueryInRequestPath, because Path may have other interpretations
• We should bump the major version number, and make the default false

I think the XDOC for the property might also read more like: Include the full URL query string in the <c>RequestPath</c> property that is attached to request log events. The default is <c>false</c>.

What do you think? I'm also happy making these kinds of changes post-merge, if you are out of time for it. Thanks again!

[jarronshih]

Hi @nblumhardt thanks for another round of review. I am also on the side to change default to false. But I am not familiar with the version bumping process.
I am keeping the default to true to prevent breaking change. Feel free to change the value to false and bump the version as the following action.

I update the variable name and doc in the last commit.

[nblumhardt]

Awesome, thanks @jarronshih 👍