[BUG] Unwanted creation of `HttpContextAccessor` for enrichment
- Dominant language
- HTML
- Stars
- 137
- Forks
- 70
- PR merge metrics
- No merged PRs in 30d
Description
**ECS integration/library project(s) (e.g. Elastic.CommonSchema.Serilog)**: Elastic.Serilog.Enrichers.Web
**ECS schema version (e.g. 1.4.0)**: N/A
**ECS .NET assembly version (e.g. 1.4.2)**: N/A
**Elasticsearch version (if applicable)**: N/A
**.NET framework / OS**: All
**Description of the problem, including expected versus actual behavior**:
Your docs at https://github.com/elastic/ecs-dotnet/blob/main/src/Elastic.Serilog.Enrichers.Web/README.md suggest the following:
```
.UseSerilog((ctx, config) =>
{
// Ensure HttpContextAccessor is accessible
var httpAccessor = ctx.Configuration.Get();
config
.Enrich.WithEcsHttpContext(httpAccessor);
```
Yet `ctx.Configuration.Get()` is intended for binding configuration. `HttpContextAccessor` is not config.
In other samples, and implied via the `// Ensure HttpContextAccessor is accessible` comment, the use of the following is suggested so `HttpContextAccessor` can be resolved via your configuration get call:
```
builder.Services.AddHttpContextAccessor();
```
These are two completely different things and your recommended code is misleading and causes unwanted behaviour.
Using the `AddHttpContextAccessor` helper to register a singleton instance of `IHttpContextAccessor` means the service container will be in control of the instance that is instantiated. Therefore, it will also dispose cleanly if needed (I appreciate the interface does not implement `IDisposable` in this case).
Accessing the `HttpContextAccessor` in the way you are recommending it be done leads to an additional instance being instantiated. It is logically equivalent to suggesting to use `new HttpContextAccessor()` instead as all that happens is `Activator.CreateInstance` is called.
Is there a reason you're doing it in this way that I have missed?
Otherwise I would suggest using your main `UseSerilog` overload instead that would look like as follows:
```
.UseSerilog((ctx, services, config) =>
{
// Ensure HttpContextAccessor is accessible
var httpAccessor = services.GetRequiredService();
config
.Enrich.WithEcsHttpContext(httpAccessor);
```
Here you are resolving the `IHttpContextAccessor` instances correctly.
Contributor guide
Assessment
This issue has not been assessed yet.