Skip to content
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

Closed
sirajmansour opened this issue Mar 14, 2018 · 28 comments
Assignees
Labels
affected-few This issue impacts only small number of customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating. severity-minor This label is used by an internal tool
Milestone

Comments

@sirajmansour
Copy link

I am facing the same dilemma as aspnet/KestrelHttpServer#2153.

Setting the trace identifier on the HttpContext isn't particularly helpful for logging because the request start scope was started before any middleware is called, so all logs use the traceidentifer set by Kestrel.

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 of Request-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 for Request-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 or RequestId - for 2.0 - which has the value of the httpcontext.TraceIdentifier that we manually set from our x-correlation-id header.

@davidfowl
Copy link
Member

Is your goal to add extra state to the logging scope or something else?

@sirajmansour
Copy link
Author

Well the state already exists, it is being defaulted in the hosting scope to CorrelationId (or RequestId in 2.0) but it is either generated by kestrel purely, or read from a set-in-stone header which is Request-Id. I'm suggesting that becomes more flexible and configurable.

@sirajmansour
Copy link
Author

sirajmansour commented Mar 14, 2018

I also don't want to widen this discussion a lot, but i've read here that the Request-Id header is used to flow the correlation id with outgoing requests from HttpClient to correlate requests going to other micro-services, which is also very important and related to this since once again the source it reads the correlation id from is not configurable; it will always come from the Request-Id header.

@davidfowl
Copy link
Member

Well the state already exists, it is being defaulted in the hosting scope to CorrelationId (or RequestId in 2.0) but it is either generated by kestrel purely, or read from a set-in-stone header which is Request-Id. I'm suggesting that becomes more flexible and configurable.

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:

  • Ability to set the TraceIdentifier for a request
  • Ability to add extra properties in the default request logging scope created by hosting
  • Ability to add headers outgoing http client requests (all of them or just specific ones?)
  • Ability to get the "current activity" from any where in the code.

/cc @glennc

@sirajmansour
Copy link
Author

sirajmansour commented Mar 14, 2018

The main use cases are:

  • Logging the correlation id coming from the headers (or any other configurable source) using the default request logging scope created by hosting. whether we do that by allowing people to add a new property in that scope or configure the name of an existing one this is an implementation detail i guess.

  • Add the captured correlation-id to a header of outgoing http client requests.

@sirajmansour
Copy link
Author

@davidfowl @glennc what are your thoughts on this ?

@ngbrown
Copy link

ngbrown commented Apr 12, 2018

I'm here because I was setting context.TraceIdentifier in a middleware, but it was always missing the very first "Request starting GET /somepath" log message because that is outputted before the middleware pipeline starts. This was setting the RequestId scope for all the rest of the log messages based on an incoming header from a preceding service (API Gateway).

I see that in ASP.Net Core 2.1 preview, the log message scope will add an CorrelationId, but it doesn't appear that the rest of the code can easily access the correlation id or that it is set if the incoming request is not setting a Request-Id header.

So, the difference between RequestId and CorrelationId should be clarified, and there needs to be a single scope key that can span multiple layers of services. I think this means that both are always set in the logging scope and CorrelationId defaults to be the same as RequestId if there is no incoming Request-Id header.

I don't mind setting outgoing HTTP calls with my own headers, but with the above logic, it would be context.TraceIdentifier only if the Request-Id header isn't set in the request.

@davidfowl
Copy link
Member

Logging the correlation id coming from the headers (or any other configurable source) using the default request logging scope created by hosting. whether we do that by allowing people to add a new property in that scope or configure the name of an existing one this is an implementation detail i guess.

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".

Add the captured correlation-id to a header of outgoing http client requests.

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.

I don't mind setting outgoing HTTP calls with my own headers, but with the above logic, it would be context.TraceIdentifier only if the Request-Id header isn't set in the request.

@ngbrown have you read https://github.com/dotnet/corefx/blob/c082d21361608e3cc2ea3cddd90c2a0306828002/src/System.Net.Http/src/HttpDiagnosticsGuide.md.

@sirajmansour
Copy link
Author

@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.

@davidfowl
Copy link
Member

Oh yeah I agree it really should be configurable.

@ngbrown
Copy link

ngbrown commented Apr 12, 2018

@ngbrown have you read ...HttpDiagnosticsGuide.md.

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 CorrelationId for logging. But it is set so early in the chain (before middleware) there's not a clean way to set it if there's no incoming header.

Thus, it would make sense for the CorrelationId to default to something such as the RequestId, or even a GUID. Or make a service point that can generate them (always, or maybe if the Request-Id header isn't set).

@sirajmansour
Copy link
Author

@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 - Request starting at /somepath - is read from a configurable source without having to implement a custom HttpContextFactory.

@davidfowl
Copy link
Member

We're going to tackle this properly in 3.0

@aspnet-hello aspnet-hello transferred this issue from aspnet/Hosting Dec 18, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0 milestone Dec 18, 2018
@aspnet-hello aspnet-hello added area-hosting enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Dec 18, 2018
@muratg
Copy link
Contributor

muratg commented Jan 9, 2019

@davidfowl, tentatively assigning this to you.

@analogrelay
Copy link
Contributor

Epic #8924

@davidfowl davidfowl added the Needs: Design This issue requires design work before implementating. label Apr 1, 2019
@analogrelay
Copy link
Contributor

@lmolkova will investigate this further and break down into specific work items for ASP.NET Core.

@davidfowl
Copy link
Member

With the way we have things planned currently:

  • TraceIdentifier is always created by the server, it's settable but it's pretty hard to plug in early enough to affect the logging scope (which I assume is the goal). It will continue to be added to the request log scope as "RequestId".
  • When the Activity is active (this will be on by default in 3.0 if logging is on), it will generate an Id on every request and it will set the parent id using the using the "Request-Id" header (legacy) or the new "traceparent" header (from the W3C trace-context format (see https://w3c.github.io/trace-context/)).

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:

  1. The incoming header that represents the "parent id"
  2. Control over changing the "current id" for the request.

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".

@analogrelay analogrelay modified the milestones: 3.0.0, Backlog Apr 15, 2019
@sirajmansour
Copy link
Author

When the Activity is active (this will be on by default in 3.0 if logging is on), it will generate an Id on every request and it will set the parent id using the using the "Request-Id" header (legacy) or the new "traceparent" header (from the W3C trace-context format (see https://w3c.github.io/trace-context/)).

It would be good to clarify some things and refresh my memory around this.

"It will generate an Id on every request" ?

Do you mean this will be Activity.Current.Id ? If yes, i'm assuming Activity.Current.ParentId will be set from the Request-Id or traceparent headers as you mentioned above. ?

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.

The incoming header that represents the "parent id"

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).

For outbound requests, HttpClient has built in support for creating a new activity and setting the outgoing header "traceparent".

I'm not sure i have enough context around this, i'll have to dig for an example.

@davidfowl
Copy link
Member

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).

@fazil1987
Copy link

Oh yeah I agree it really should be configurable.

@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

@davidfowl
Copy link
Member

The name isn't configurable no. Anything custom requires custom logic in HttpClient (a delegating handler) and ASP.NET Core (a middleware)

@alefranz
Copy link
Contributor

Hi @fazil1987 ,
if you are looking for the ability to pass a incoming header down to the HttpClient, you can use the HeaderPropagation middleware which does exactly that (see example).

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.

@Tratcher Tratcher added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Nov 6, 2020 — with ASP.NET Core Issue Ranking
@davidfowl
Copy link
Member

@shirhatti assigning this to you.

@davidfowl davidfowl assigned shirhatti and unassigned davidfowl Mar 23, 2021
@ghost
Copy link

ghost commented Mar 23, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@shirhatti
Copy link
Contributor

FYI @tarekgh

@davidfowl
Copy link
Member

This doesn't work as a feature btw. It happens too early

@shirhatti
Copy link
Contributor

As requested in the original issue, the use of alternative header, e.g,. X-Correlation-Id (instead of Request-Id) is now possible via the use of propagators on both ASP.NET Core and HttpClient.

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

@abatishchev
Copy link

But what about arbitrary header, e.g. x-ms-coreelation-id or x-ms-client-request-id, etc ?

@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 20, 2021
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-few This issue impacts only small number of customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating. severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests