Remove default logging scope from ASP.NET Core

[davidfowl]

• We log the activity information by default, the span id is more useful than the traceidentifier.
• Add the path and trace id as activity tags

I was looking at this and wondering why we still need it other than compatibility. I'm thinking we can lean into activity (as big as it is 😄) and use it's scope instead of having 2 by default. This reduces the number of async locals as well which is a plus. @noahfalk do you think it's reasonable to have a feature for logging adding tags to the logging activity scope as well?

cc @noahfalk @tarekgh @adityamandaleeka

These are the per request allocations (excluding execution context overhead) (10K requests):

Type	Allocations	Bytes	Average Size (Bytes)
| - Microsoft.Extensions.Logging.Logger.Scope	10,125	486,000	48
| - Microsoft.Extensions.Logging.LoggerFactoryScopeProvider.Scope	10,125	486,000	48
| - Microsoft.AspNetCore.Hosting.HostingApplicationDiagnostics.Log.HostingLogScope	10,000	400,000	40
| - System.IDisposable[]	10,125	324,000	32

[tarekgh]

Add the path and trace id as activity tags

Did you mean Request Id instead of trace id?

[tarekgh]

@davidfowl I think this is change is good if we are not concering about the compat. If we proceed with such change would be good to file breaking change doc for that.

CC @reyang @cijothomas

[noahfalk]

@noahfalk do you think it's reasonable to have a feature for logging adding tags to the logging activity scope as well?

what if we encourage people to phase out usage of logger scope and use Activity instead? My worry if we try to automatically migrate tags is that the scope lifetimes may not match up and you could have weird results. For example:

// this might be ASP.Net code
void HandleRequest()
{
    using (Activity a = source.StartActivity)
    {
        UserRequestHandler(requestData);
    }
}

void UserRequestHandler(requestData)
{
    foreach(var item in requestData.Batch)
    {
        // this scope doesn't have 1:1 relationship with Activity, the tags would overwrite each other
        using(logger.CreateScope("Id", item.Id))
        {
            logger.Log("stuff");
        }
        // this log is outside the logger scope, but still inside the Activity scope where
        // the tags have now been forwarded.
        logger.Log("all done now");
    }
}

[NinoFloris]

Relevant #31028

[davidfowl]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[amcasey]

@davidfowl What's happening with this?

[ghost]

Thanks for identifying a breaking change.

, after you commit this PR please take the following actions, as part of the breaking changes announcement process:
\n- [ ] Create an announcement issue by using the ASP.NET Core breaking change issue template.
\n- [ ] Link the breaking change announcement issue from this PR.
\n- [ ] Remove the needs-breaking-change-announcement label.

[davidfowl]

@noahfalk I updated this PR to just remove the scopes.

I'm pretty convinced this scope useful by default and can be easily added back via middleware. The cost we're paying for the benefit people get by default is too high (many don't even know how to look at these logging scopes). I think now that the ability to associate logs with the activity exists and is on by default. I feel pretty confident breaking this.

We will show people how to add a middleware back and adds this information via a logging scope or activity tags.

[dotnet-policy-service]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[Copilot]

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/Hosting/Hosting/test/WebHostTests.cs:871

• The removal of the WebHost_CreatesDefaultRequestIdentifierFeature_IfNotPresent test case reduces the test coverage. Ensure that the new behavior is adequately tested.

[Fact]

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs:95

• Ensure that the removal of the Scope property and its associated logic is covered by tests to verify that the activity's scope is used correctly.

if (loggingEnabled)