stevejgordon / CorrelationId

An ASP.NET Core middleware component which synchronises a correlation ID for cross API request logging.
MIT License
560 stars 106 forks source link

Not compatible with NetCore 2.2 #34

Closed sophisma closed 5 years ago

sophisma commented 5 years ago

I just updated an MVC Project from Net Core 2.1 to 2.2 and noticed an issue with this nuget. The thing is that in one of my classes I have a dependency on HttpContextAccessor, and so I registered it in the ConfigureServices method:

services.AddHttpContextAccessor();

After this I register this nuget: services.AddCorrelationId();

And the initialize it in the Configure method: app.UseCorrelationId(new CorrelationIdOptions { Header = "X-Correlation-ID", UseGuidForCorrelationId = true }); When I try to use the injected HttpContentAccessor instance in the following manner:

var accessToken = httpContextAccessor.HttpContext.GetTokenAsync("access_token").Result;

I get a null exception on the HttpContext class. If I remove the nuget from the ConfigureServices and Configure methods the class has a value. Could you add Net Core 2.2 support?

stevejgordon commented 5 years ago

@sophisma I've been using CorrelationId in an ASP.NET Core 2.2 project with no issues so far although I don't register the HttpContextAccessor.

When accessing the correlation ID you should be using the CorrelationContextAccessor which is not linked to the HttpContext.

Are you able to provide a minimal reproduction sample so I can investigate further?

BrianBroe commented 5 years ago

I no longer get the RequestId field updated with the correlation ID in my Serilog loggings after updating to Core 2.2. Do I need to change something? Or make my own middleware for setting the log context?

sophisma commented 5 years ago

@stevejgordon when I updated my solution I had two issues and I ended up mixing them up. There's nothing wrong with your code. I'm really sorry.

stevejgordon commented 5 years ago

@sophisma No worries at all! :-)

Hafeed3s commented 5 years ago

@sophisma Can you please detail what was the issues? I have exactly the same problem, everything was working perfectly until I migrated to .net core 2.2, now I have my httpContextAccessor.HttpContext which is always null

sophisma commented 5 years ago

Sorry to get back to this, I've had to deal wit a lot of stuff migrating from older versions of netcore to netcore 2.2 and I got myself distracted.

Maybe this should be reopened?

When you register CorrelationId, somehow it causes IHttpContextAccessor.HttpContext to become null. I'll post an exemple project showing this in a few minutes.

sophisma commented 5 years ago

You can check this issue happening in this Project:

https://github.com/sophisma/CorrelationIdIHttpContextAccessorTest.git

Just to give a bit of context, no pun intented, the HomeController depends on a service named "SomeService", which in turn depends on IHttpContextAccessor. When the Index action is called on the HomeController the following method is executed:

ViewBag.Context = this.someService.SomeMethod();

This method just returns the HttpContext.Headers values to the controller. In a first scenario I left the CorrelationId setup lines commented and everything works fine. As soon as I initialize CorrelationId I get a null HttpContext. If you need any clarification I'll be happy to explain.

Edit This issue was closed, I don't know if @stevejgordon will get a notification for this. If needed I'll create a new issue, tomorrow.

Hafeed3s commented 5 years ago

@sophisma I'm exactly in the same situation, if I comment app.UseCorrelationId() and services.AddCorrelationId() everything works ok (HttpContext is not null)

@stevejgordon this is what I do in my Startup.cs

        public void ConfigureServices(IServiceCollection services)
        {
            services.AddMediatR();

            services.AddLogging(
                loggingBuilder =>
                {
                    loggingBuilder.AddSerilog(dispose: true);
                    loggingBuilder.AddDebug();
                });

            services
                .AddMvc()
                .SetCompatibilityVersion(CompatibilityVersion.Version_2_2)
                .AddJsonOptions(options => { options.SerializerSettings.DateTimeZoneHandling = DateTimeZoneHandling.Utc; });

            services.AddCorrelationId();
...

and

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }
            else
            {
                app.UseHsts();
            }

            app.UseCorrelationId(new CorrelationIdOptions() { UseGuidForCorrelationId = true });
...

Any reason why HttpContext would be null when using correlation id ?

stevejgordon commented 5 years ago

@sophisma / @Hafeed3s - Sorry for the delay. I've dug into this and narrowed it down to cases where the TraceIdentifier on the context is updated to include the Correlation ID (the default behaviour)

On investigation, this appears to be a regression (see https://github.com/aspnet/AspNetCore/issues/5144) with ASP.NET Core, where changing the TraceId in 2.2 causes the HttpContext to become null.

A fix is in place for 3.0 and is being back-ported for inclusion in a patch of 2.2.x, likely 2.2.2 since 2.2.1 is already closed.

A workaround at this point is to disable the behaviour of updating the TraceIdentifier using the options when adding the middleware...

app.UseCorrelationId(new CorrelationIdOptions
{
    Header = "X-Correlation-ID",
    UseGuidForCorrelationId = true,
    UpdateTraceIdentifier = false
});
sophisma commented 5 years ago

Thank you @stevejgordon for your patience.

RehanSaeed commented 4 years ago

3.0 is out now. Any chance of an update @stevejgordon? I'd really like the AddCorrelationId method to return IServiceCollection.