-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a way to configure the IHttpRequestIdentifierFeature to read a configurable header #5914
Comments
Is your goal to add extra state to the logging scope or something else? |
Well the state already exists, it is being defaulted in the hosting scope to |
I also don't want to widen this discussion a lot, but i've read here that the |
But that's not what I asked. I'm asking what you specifically are looking for. I'm aware of what our current infrastructure does but it may not work as you'd expect. All of that logic doesn't just work, it requires a diagnostic listener to be wired up which may be undesirable. I'd like to understand what your trying to achieve (I have some ideas but I don't want to assume anything). Here are some of the things I can think of:
/cc @glennc |
The main use cases are:
|
@davidfowl @glennc what are your thoughts on this ? |
I'm here because I was setting I see that in ASP.Net Core 2.1 preview, the log message scope will add an So, the difference between I don't mind setting outgoing HTTP calls with my own headers, but with the above logic, it would be |
We do this by default in 2.1. If you set the "Request-Id" header (not configurable yet). It gets stamped onto the logging scope as "CorrelationId".
That's where this currently comes in https://github.com/dotnet/corefx/blob/c082d21361608e3cc2ea3cddd90c2a0306828002/src/System.Net.Http/src/HttpDiagnosticsGuide.md. Basically the right things for outgoing requests light up automagically if you add a diagnostic source subscription.
@ngbrown have you read https://github.com/dotnet/corefx/blob/c082d21361608e3cc2ea3cddd90c2a0306828002/src/System.Net.Http/src/HttpDiagnosticsGuide.md. |
@davidfowl yes i know it is. But my point is about making it configurable, such as if you set "MyWeirdCustomHeader" header, it gets stamped onto the logging scope as "MyWeirdCustomCorrelationId" if i have that configuration in place. |
Oh yeah I agree it really should be configurable. |
Yes, it doesn't mention anything about a browser client hitting the first service. If a request comes in to an API gateway and is scattered/gathered or even goes through multiple layers, that first endpoint hit as the gateway should be included in the Thus, it would make sense for the |
@davidfowl cool, this pretty much sums up what'd be nice to see in a future release. That and a change that ensures having the logging scope of the very first request log - |
We're going to tackle this properly in 3.0 |
@davidfowl, tentatively assigning this to you. |
Epic #8924 |
@lmolkova will investigate this further and break down into specific work items for ASP.NET Core. |
With the way we have things planned currently:
This means we'll have 2 properties on the request logging scope by default ("RequestId" and "ActivityId"). Coming back to the original request from @sirajmansour: As for configurability of the above, it seems there are 2 things that could be configurable:
We don't allow you to reasonably change any of those today but it would be good to understand what you need to change here. We also currently don't have a "passthrough" concept where the "parent id" and "current id" are the same, there's always a hierarchy. What do you want to configure and where do you want it to go? For outbound requests, HttpClient has built in support for creating a new activity and setting the outgoing header "traceparent". |
It would be good to clarify some things and refresh my memory around this.
Do you mean this will be And i assume you are referring to those two further down as "current id" and "parent id" ? Basically I think what i want to be configurable hasn't changed.
This seems to tick one of the boxes. The other thing, would be to control how this incoming header value affects the logging scope (iow, how it appears in the logs).
I'm not sure i have enough context around this, i'll have to dig for an example. |
We’ve punted this issue from as we’re focusing on supporting w3c and having it on by default. That means if you set the incoming request id, or trace parent it’ll be the parent Id and if logging is on, the scope will contain both trace identifier as RequestId and ActivityId as the full ActivityId (including parent id). Now that we have a standard for these things id expect people to trend that direction (traceparent being the standard header and traceid and span is being the standard properties that appear in logs). |
@davidfowl so is Request-Id header name currently configurable ? if so, how do i do that as we are not planning to move to w3c standard any time soon. Please reply |
The name isn't configurable no. Anything custom requires custom logic in HttpClient (a delegating handler) and ASP.NET Core (a middleware) |
Hi @fazil1987 , For the logging part, you can create a middleware at the start of the pipeline to get the header and add it to the logger scope. |
@shirhatti assigning this to you. |
Thanks for contacting us. |
FYI @tarekgh |
This doesn't work as a feature btw. It happens too early |
As requested in the original issue, the use of alternative header, e.g,. See dotnet/runtime#50658 and #33777 for more details. As such I'm closing this issue. Please feel free to open a new issue if propagators does not work for you and we'd be happy to discuss this in greater detail |
But what about arbitrary header, e.g. |
I am facing the same dilemma as aspnet/KestrelHttpServer#2153.
Basically, we force all incoming requests to our services to have a correlation-id header as we want to track the whole flow coming from the front-end or any other consumers of our APIs. We also prefer using
X-Correlation-Id
instead ofRequest-Id
.Although it seems a workaround is mentioned there by @davidfowl, i think it is valuable having that option configurable at the early stages of creating the webhost.
It would great to be able to read from a header or (any other delegate/provider). And propagating that change across to
HostingApplicationDiagnostics.cs
instead of setting it always to look forRequest-Id
header.Also making the property name in the log name configurable is valuable too as it is also statically set to
CorrelationId
as shown in https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNetCore.Hosting/Internal/HostingLoggerExtensions.cs orRequestId
- for 2.0 - which has the value of the httpcontext.TraceIdentifier that we manually set from our x-correlation-id header.The text was updated successfully, but these errors were encountered: