elastic / elastic/ecs-dotnet

[BUG] Unwanted creation of `HttpContextAccessor` for enrichment

Open
#471 0 comments 2 reactions 0 assignees View on GitHub
bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.