Skip RequestPath property

[andreycha]

Request logging middleware from Serilog.AspNetCore adds RequestPath property to log events.

This PR skips RequestPath property, it's already mapped to url.path if MapHttpAdapter is enabled (which is a preferred way of getting all the HTTP-related information in the log entry): https://github.com/elastic/ecs-dotnet/blob/main/src/Elastic.CommonSchema.Serilog/Http/HttpContextAccessorAdapter.cs#L108

[cla-checker-service[bot]]

💚 CLA has been signed

[apmmachine]

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-09-02T10:02:41.030+0000

• Duration: 16 min 15 sec

Test stats :test_tube:

Test	Results
Failed	0
Passed	100
Skipped	2
Total	102

:robot: GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[andreycha]

As I dug a little bit deeper into the topic, this PR seems to be incomplete and to require discussion.

Both Serilog.AspNetCore and standard AspNet.Core loggers enrich events with request-related information which currently ends up in metadata. By defaults it's not propagated to standard ECS fields. Once user configures MapHttpAdapter, some of this information appears in ECS fields (and becomes redundant in metadata).

Here is the summary (probably still incomplete):

Serilog.AspNetCore enriches events with:

Property	ECS field	Is mapped
RequestMethod	http.request.method	When MapHttpAdapter is configured
RequestPath	url.path	When MapHttpAdapter is configured
StatusCode (only in responses)	http.response.status_code	When MapHttpAdapter is configured
Elapsed (only in responses)	event.duration (with conversion to ns)	No

Microsoft loggers enrich events with:

Property	ECS field	Is mapped
Protocol	N/A (if parsed out, can be put into http.version)	No
Method	http.request.method	When MapHttpAdapter is configured
Scheme	url.scheme	When MapHttpAdapter is configured
PathBase	N/A	N/A
Path	url.path	When MapHttpAdapter is configured
QueryString	url.query	When MapHttpAdapter is configured
RequestId	http.request.id	No
ElapsedMilliseconds (only in responses)	event.duration (with conversion to ns)	No
ContentType (only in responses)	http.response.mime_type	No
StatusCode (only in responses)	http.response.status_code	When MapHttpAdapter is configured

I guess there are multiple options of how Elastic.CommonSchema.Serilog might behave:

1. If MapHttpAdapter is not configured, populate http and url fields from request-related log properties.
2. If MapHttpAdapter is configured, let it populate http and url fields.
3. Merge together information available in log properties and provided by http adapter (if configured). This might be the best option because it will also cover fields which are currently not mapped.

In all cases it probably makes sense to not duplicate request-related properties in metadata as it's already the case with other properties and enrichers.

Also the part with Microsoft loggers seems to be applicable to Elastic.CommonSchema.NLog.

[andreycha]

Done.

Should I try to implement and open a PR with more holistic approach that covers other things mentioned above?

[Mpdreamz]

@andreycha that would be great! I wonder why we don't assign our HttpAdapter always?

Elastic.CommonSchema.NLog and Elasticsearch.Extensions.Logging will need similar treatment but lets address those in a follow up PR.

[Mpdreamz]

Elastic.CommonSchema.NLog is addressed by https://github.com/elastic/ecs-dotnet/pull/195/files

[andreycha]

Alright, I will then prepare a separate PR. The only thing that I'm concerned about is that some of these property names are quite generic (e.g. Path) and generally can be used outside of HTTP request scope and/or web application, yet will end up in http-related field...

I wonder why we don't assign our HttpAdapter always?

It would be nice to have it always mapped but when should the library take IHttpContextAccessor from? And what if the host application is not a web application?

[andreycha]

Closing this since https://github.com/elastic/ecs-dotnet/pull/213 has been merged.