Runtime: Determine how to handle some System.Net logging methods
We are missing implementations for several methods on NetEventSource: PrintInfo, PrintWarning, etc.
We need to figure out how we plan to handle this. Either implement the methods, or change the calling code to do something else.
geoffkizer
All 18 comments
cc: @CIPop
davidsh
on 16 Oct 2016
Should we make it part of netstandard2.0?
karelz
on 17 Oct 2016
Should we make it part of netstandard2.0?
This is an internal implementation detail; it's not something specified in the standard.
stephentoub
on 17 Oct 2016
This is an internal implementation detail; it's not something specified in the standard.
Understood. But if the original implementation has it and it is useful and we do not bring it over, then it is a debt we are creating and we should track it as part of netstandard2.0. We may triage it post 1.2, but we should just not pile up more debt than we have to.
karelz
on 17 Oct 2016
we should track it as part of netstandard2.0. We may triage it post 1.2, but we should just not pile up more debt than we have to.
Oh, you're using the terms "netstandard2.0" and ".NET Core 1.2" interchangeably? I view those as being separate; the former is a specification for APIs that need to be available, and the latter is a particular implementation of that standard. This work item is relevant to the latter but not to the former, so if your question is "should we track this for .NET Core 1.2", I'd say "yes".
cc: @weshaggard
stephentoub
on 17 Oct 2016
Oh, you're using the terms "netstandard2.0" and ".NET Core 1.2" interchangeably?
Not really, but we may have different definition of nestandard2.0 here.
I treat it as a label in CoreFX which is about implementation of the .NET Standard 2.0 in .NET Core code base. It comes with API surface, its implementation, tests and everything else (incl. necessary useful logging). Unless we explicitly wave something out during the porting -- it can be parts of API surface, their implementation (stubbing out), or tests, or parts of implementation like logging -- it should be labeled with netstandard2.0, so that we do not lose track of it and don't falsely assume that "we are done", while we created tons of backlog/debt no one is funded to finish off ... (modulo exceptions of course)
cc: @danmosemsft for his take
karelz
on 17 Oct 2016
If it's part of bringing over a functional, tested type to satisfy NS2.0 surface area, it's worth labeling netstandard2.0. Then we can truthfully say that when there are no more issues labeled as such, we have cleared all the debt we introduced in the effort. If it's a marginal part of the implementation, or an optional enhancement to it, or a bug that already existed, ... then not. That's just my take.
danmosemsft
on 17 Oct 2016
Logging is part of .Net Desktop behavior: https://msdn.microsoft.com/en-us/library/ty48b824(v=vs.110).aspx
For debugging purposes, logging is indispensable for understanding why a state machine fails. That said, it's not critical for apps and the mainstream scenarios to work.
We need to figure out how we plan to handle this. Either implement the methods, or change the calling code to do something else.
@geoffkizer We already designed and are following a pattern for all System.Net: dotnet/corefx#5101. We're converting all public logging into EventSource messages with arguments for the public logs. We're enabling GlobalLog and keeping those as free-form strings.
CIPop
on 20 Oct 2016
@CIPop Yes, I know about NetEventSource. The problem is that it's missing a few of the old Logging methods that FtpWebRequest uses (and I suspect, other system.net code too). Is the solution to just add new events?
geoffkizer
on 20 Oct 2016
We already designed and are following a pattern for all System.Net
It's also broken in many cases due to reusing the same event sources in multiple libraries, and the pattern leads to very verbose code at the call sites, as well as adding significant inefficiencies when tracing is enabled. It all needs to be revisited.
stephentoub
on 20 Oct 2016
Is the solution to just add new events?
The current pattern is to add new events with unique IDs for unique strings, following the EventSource guidance.
It's also broken in many cases due to reusing the same event sources in multiple libraries
Yes, this is known and tracked by dotnet/runtime#16239.
the pattern leads to very verbose code at the call sites
@vancem Is this a known issue with EventSource? @stephentoub are we using the EventSource incorrectly in System.Net code?
adding significant inefficiencies when tracing is enabled
@stephentoub Please add a tracking item with more details.
CIPop
on 20 Oct 2016
@vancem Is this a known issue with EventSource? @stephentoub are we using the EventSource incorrectly in System.Net code?
This has nothing to do with EventSource itself, but rather the pattern that's being used by the networking libs. The call sites are making multiple calls for each call site (one for event source, one for asserts), and they're doing a lot of computation in the calls, which forces them to be guarded by an IsEnabled check. What should be one line often ends up being ~10.
stephentoub
on 20 Oct 2016
The call sites are making multiple calls for each call site
This was considered during design. We have two options: change logging to only exposed already available data or maintain log compatibility with .Net Desktop. At that time we've chosen to go with option 2. What is your recommendation?
CIPop
on 20 Oct 2016
What is your recommendation?
If there's a requirement that every SomeEventSource.Assert call has a corresponding Debug.Assert call, and vice versa, then a single entrypoint should be defined somewhere that does both. And for efficiency, the call site should do minimal work, relying on the called function to do that work behind a guard check for IsEnabled, such that the call site doesn't need an IsEnabled check either. So for example, code like:
``` C#
if (GlobalLog.IsEnabled)
{
GlobalLog.AssertFormat("Some {0} message", LoggingHash.HashString(this));
}
Debug.Fail("Some " + LoggingHash.HashString(this) + " message");
becomes:
``` C#
GlobalLog.Log.Fail("Some {0} message", this);
with Fail defined along the lines of:
C#
public void Fail<T>(string formatMessage, T arg)
{
if (IsEnabled())
{
FailEvent(string.Format(formatMessage, arg));
. }
Debug.Fail(string.Format(formatMessage, arg));
}
etc.
stephentoub
on 20 Oct 2016
The work is tracked by dotnet/runtime#16054.
CIPop
on 20 Oct 2016
cc @Priya91, @ericeil -- I expect they've been dealing with this issues too.
geoffkizer
on 20 Oct 2016
Thanks. (And to be clear, my comments in that thread were for the short term solution, as a way to not do a ton of allocation while not redesigning the pattern. But we should redesign the pattern :)
stephentoub
on 20 Oct 2016
Since I've been complaining about the System.Net logging and the issues have been sitting here for so long, I've assigned them to myself and am working on an overhaul.
stephentoub
on 24 Oct 2016
Related issues
yahorsi
路
3Comments
bencz
路
3Comments
omariom
路
3Comments
jkotas
路
3Comments
btecu
路
3Comments
Most helpful comment
If there's a requirement that every SomeEventSource.Assert call has a corresponding Debug.Assert call, and vice versa, then a single entrypoint should be defined somewhere that does both. And for efficiency, the call site should do minimal work, relying on the called function to do that work behind a guard check for IsEnabled, such that the call site doesn't need an IsEnabled check either. So for example, code like:
``` C#
if (GlobalLog.IsEnabled)
{
GlobalLog.AssertFormat("Some {0} message", LoggingHash.HashString(this));
}
Debug.Fail("Some " + LoggingHash.HashString(this) + " message");
with Fail defined along the lines of:
C# public void Fail<T>(string formatMessage, T arg) { if (IsEnabled()) { FailEvent(string.Format(formatMessage, arg)); . } Debug.Fail(string.Format(formatMessage, arg)); }etc.