Use skipEnabledCheck for LoggerMessage.Define

[gfoidl]

Addresses #31332

For Kestrel / Transports 43d331a moves the IsEnabled check to the *Trace-class, so the usage is more streamlined, and the responsibility for logging is solely in one place. I.e. the IsEnabled checks aren't sprinkled around.
But I'm not entirely sure if this approach is correct, as on regards of arguments it got less specific (no explicit connectionId argument).

---

In ad264bb9ed72cd3a9250c5e3f0c5f0a2da33ce74 (not part of this PR) I tried to get rid of the connection id as much as possible, to avoid the allocation of that string when not needed.
This commit isn't in a working state, as Http2StreamContext passed null for the ConnectionContext, thus the approach won't work with some other changes, which we should discuss first.

• So is this something to follow / is this a goal to achieve?
• @davidfowl from your motivating example in Add LoggerMessage.Define overload to disable IsEnabled check runtime#45290 have you already planned some work?

[davidfowl]

@gfoidl did you check all the places to make sure they have if (IsEnabled())) checks? I assume yes but they don't show up in the code review obviously so it's hard to know without seeing a perf regression 😄

[gfoidl]

did you check all the places to make sure they have if (IsEnabled())) checks?

Yeah, I checked the callsites (and worked backwards to the logger definitions).
Callsites found by regex-search (git + VS).

[davidfowl]

For Kestrel / Transports 43d331a moves the IsEnabled check to the *Trace-class, so the usage is more streamlined, and the responsibility for logging is solely in one place. I.e. the IsEnabled checks aren't sprinkled around.
But I'm not entirely sure if this approach is correct, as on regards of arguments it got less specific (no explicit connectionId argument).

I would love to move Kestrel away from these interfaces in general but that would mean updating the tests 😄. @halter73 , do you have strong opinions here? (I do...)

[halter73]

@halter73 , do you have strong opinions here? (I do...)

Those logging interfaces are a carryover from before LoggedTest.TestSink was a thing. It's a good idea to move all our tests that rely on mocking the IKestrelTrace and ISocketTrace interfaces to just use that.

I do like having all well-defined Kestrel logs in one place even if we get rid of the interfaces, and I don't think we need to remove the interfaces in this PR since that would require a lot of test updates.

[JamesNK]

@halter73 , do you have strong opinions here? (I do...)

Those logging interfaces are a carryover from before LoggedTest.TestSink was a thing. It's a good idea to move all our tests that rely on mocking the IKestrelTrace and ISocketTrace interfaces to just use that.

I do like having all well-defined Kestrel logs in one place even if we get rid of the interfaces, and I don't think we need to remove the interfaces in this PR since that would require a lot of test updates.

#31581