Parent Request Id is mislabeled as Correlation Id in log scope

[ngbrown]

For the upcoming for ASP.Net Core 2.1; pull #1138 added to the log scope a CorrelationId, but the contents are only the parent request identifier and does not appear to be set if there was no parent request (no Request-Id header in the incoming HTTP request).

There is documentation/specifications (mostly by @lmolkova) around these terms at:

• HttpCorrelationProtocol
• FlatRequestId
• HierarchicalRequestId

Specifically in the Activity User Guide, it says:

When application calls external dependency to complete an operation, it may need to pass some of the context (e.g. correlation id) [emphasis added] along with dependency call to be able to correlate logs from multiple service.

I see that the expectation is that in the logs, there will be a "correlation id" of the context, or CorrelationId if you will, that can be used to link the logs entries across multiple services handling a single user request.

In the current preview code, the Log scope outputs a CorrelationId which is the value of an incoming header Request-Id, and only if that header is set. This is not useful for seeing the entire request across multiple services in the logs. What is needed is adding to the log scope the root request id to the log scope of all services involved in handling the request, including within the root service that generated the first request id.

[lmolkova]

@ngbrown
The spec implemented in .NET and ASP.NET is HierarchicalRequestId

i.e. 'correlationId' looks like |root-id-that-never-changes.some_unique.suffix.1.2.3.

You are right that 'parent request id' is the correct name for it.

Instead of adding parent Request-Id header value to logger scopes, we should use Activity.Current.Id that is generated for each request even if there were no Request-Id header.

I believe @davidfowl hit a snag there because no instrumentation (i.e. Activity creation) happens in absence of ApplicaitonInsights or other tracing systems that lights-up hosting diagnostics.

[ngbrown]

If CorrelationId in the log entries would most correctly be the Activity.Current.Id, then it could be conditionally added, depending on whether or not the Activity is created.

If that can't be achieved, then I don't think it should be set with the current name at all. i.e. change the log scope/enrichment name to ParentRequestId. There are also parameter names that should be updated.

[davidfowl]

@lmolkova is right. Maybe we should just remove this from the logging scope altogether.

[ngbrown]

Another issue is that the log context RequestId in an hierarchical request id scenario does not include any part of the hierarchy.

The spec says:

"All operation logs may be queried by Request-Id prefix |Guid., logs for particular request may be queried by exact Request-Id match"

[muratg]

@davidfowl anything to do in 2.2?

[davidfowl]

Yes, we should look at this in 2.2 and also look at disabling this by default because of the other security concerns raised here aspnet/Hosting#1385

[muratg]

@Tratcher thoughts?

[Tratcher]

No specific thoughts, I wasn't involved in most of this work. @pakrym?

[muratg]

@davidfowl Moving this out to 3.0.0.

[muratg]

@pakrym thoughts on the scope of this one? You agree that removing it from logging scope is the right approach?

[pakrym]

I, unfortunately, don't have enough context on this. Would need to discuss with @davidfowl as it seems he understands what's wrong here.

[lkts]

I want to agree here that CorrelationId is confusing in current implementation because it is always null for application which serves "initial" requests. I would expect it to be Activity.Current.Id as mentioned here.

[analogrelay]

Epic #8924

[analogrelay]

Needs similar design to #5926

[davidfowl]

This is done now

[davidfowl]

See #9491