Skip to content

Remove IKestrelTrace #37242

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.

Sign up for GitHub

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

Jump to bottom
Merged
JamesNK merged 8 commits into from
Oct 6, 2021
Merged

Remove IKestrelTrace #37242

JamesNK merged 8 commits into from
Oct 6, 2021

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 4, 2021 •
edited
Loading

Fixes #31581

Yeet IKestrelTrace and associated implementations.

TODO:

  • Seal KestrelTrace
  • Remove virtual methods from KestrelTrace
  • Remove commented out files

I will do the same for SocketsTrace and QuicTrace in future PRs.

@davidfowl
Copy link
Member

davidfowl commented Oct 4, 2021

Why do we still have kestrel trace?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 4, 2021 •
edited
Loading

It wraps a bunch of differently named loggers:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

Lines 13 to 28 in cd8e890

internal partial class KestrelTrace : IKestrelTrace
{
protected readonly ILogger _generalLogger;
protected readonly ILogger _badRequestsLogger;
protected readonly ILogger _connectionsLogger;
protected readonly ILogger _http2Logger;
protected readonly ILogger _http3Logger;
public KestrelTrace(ILoggerFactory loggerFactory)
{
_generalLogger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel");
_badRequestsLogger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel.BadRequests");
_connectionsLogger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel.Connections");
_http2Logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel.Http2");
_http3Logger = loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel.Http3");
}

It could be split out into BadRequestsLog, Http2Log, etc, but I think a centralized type that ensures the right logger is used for each log message seems worthwhile.

Tratcher
Tratcher approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy to see these test trace classes gone.

@JamesNK JamesNK merged commit 6c5e668 into main Oct 6, 2021
@JamesNK JamesNK deleted the jamesnk/kestreltrace branch October 6, 2021 17:47
@ghost ghost added this to the 7.0-preview1 milestone Oct 6, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Tratcher approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

7.0-preview1

Development

Successfully merging this pull request may close these issues.

Remove IKestrelTrace

5 participants

@JamesNK @davidfowl @halter73 @Tratcher @amcasey