stevejgordon / CorrelationId

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

Preserving RequestId? #13

Closed tdabasinskas closed 6 years ago

tdabasinskas commented 6 years ago

Hi,

It seems that if I use this library, the RequestId is getting replaced with theCorrelationId, retrieved/generated by this library.

Is there a way to use this library, but still preserve the default RequestId? Our API need both.

Thank you.

stevejgordon commented 6 years ago

@TDabasinskas Sorry for the delay. GitHub notifications never seem to reach me, despite me owning the repository!

That was a design decision I took since that ensures the same ID is used with the built in ASP.NET Core logging also and everything can be tied together. That said, I could probably make that configurable for cases such as yours.

Are you able to provide more details as to why you need the RequestId? I'm interested to understand the use cases for people using this library.

tdabasinskas commented 6 years ago

Hello, @stevejgordon,

No problem. Thanks for getting back to me.

The case is quite straightforward: as per our API guidelines, we use RequestId to identify individual requests to the service, and use CorrelationId to identify the cross-service requests. Your library covers the latter case, but, while doing that, it breaks the former functionality, as it's no longer possible to identify (i.e. log) the unique request ID.

In addition, the logging in ASP.NET Core now logs actual correlation ID as RequestId, which is not quite correct.

Not sure if I'm being clear (or maybe I'm doing something wrong), but looking forward for any further feedback.

stevejgordon commented 6 years ago

That makes sense and is a case I hadn't considered. I've got a few tweaks I'm looking to bring in when I can get some time on this so I will include this as an item.

stevejgordon commented 6 years ago

@TDabasinskas Can I please verify how you are currently accessing RequestId to ensure we're talking about the same thing?

stevejgordon commented 6 years ago

@TDabasinskas I've pushed an RC package for 2.1 which includes two extra configuration options. One of those is called UpdateTraceIdentifier. This defaults to true but you can try setting this to false which should give you the behaviour you want.

app.UseCorrelationId(new CorrelationIdOptions {
    UpdateTraceIdentifier = false
});

If you could let me know if that works as you expected that would be great. I'll aim to release a final version of 2.1 after I've had chance to pull it into a real project myself.

You should be able to add the package using dotnet add package CorrelationId --version 2.1.0-rc

tdabasinskas commented 6 years ago

Hi, @stevejgordon,

Thanks!

It seems to be working as expected if CorrelationId is sent via the header. Anyhow, if I use the following code to setup the middleware and send a request with no CorrelationId header, the application crashes with the exception provided below.

Middleware:

.UseCorrelationId(new CorrelationIdOptions
{
    UpdateTraceIdentifier = false,
    UseGuidForCorrelationId = true,
    Header = "Correlationid",
    IncludeInResponse = true,
})

Exception (as I understand, it's coming from here):

System.ArgumentNullException: Value cannot be null.
Parameter name: correlationId
   at CorrelationId.CorrelationContext..ctor(String correlationId, String header)
   at CorrelationId.CorrelationContextFactory.Create(String correlationId, String header)
   at CorrelationId.CorrelationIdMiddleware.<Invoke>d__3.MoveNext()

Am I setting something incorrectly? To my understanding, empty CorrelationId should not be an issue - a new GUID should be generated and assigned to it instead.

stevejgordon commented 6 years ago

@TDabasinskas Might be a bug as other than my unit tests and test project I didn't give this a full test yet. Will try to take a look later.

stevejgordon commented 6 years ago

@TDabasinskas I'm struggling to replicate this locally. I'm never able to get a null correlationId in through the header. I can put a guard around it in code though.

Are you able to provide more details about the request you're making? I'd love to replicate it before fixing it so I can be confident I've covered all of the bases.

I tried with an empty value in for the Correlationid header as well as various combinations that might be considered empty.

stevejgordon commented 6 years ago

Not to worry. With the latest version of Postman I managed to send an empty value on the Correlationid header and get it to blow. I'll put in the guard around that.

stevejgordon commented 6 years ago

@TDabasinskas Give it an hour or so and once indexed you should be able to pull RC2 which I hope fixes this bug. If that looks good I'll try to test over the weekend and get a final release out.

https://www.nuget.org/packages/CorrelationId/2.1.0-rc2

stevejgordon commented 6 years ago

In hindsight I should have called these betas!