Tracing levels and actual trace event Ids

[daviburg]

Fix event ids, they are supposed to be unique identifiers per trace event message, not a thread identifier. Implemented trace event levels ("types") Git ignore hidden VS 2019 folder. Re-ordered out of order using declarations.

[drieseng]

Most of these changes should be reverted:

• You cannot use string interpolation as we still support target framework that require us to use an old(er) C# compiler.
• I don't see why our diagnostics class should be public. You can register listeners without this.

I also don't like these "magic" event ids. Perhaps consider using an internal enum for this? I'll try to have a closer look later this week.

[daviburg]

• You cannot use string interpolation as we still support target framework that require us to use an old(er) C# compiler.

SSH .NET targets Framework 3.5 as the oldest, and all framework version are compatible with C# version 7.3 per https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version#defaults

[daviburg]

• I don't see why our diagnostics class should be public. You can register listeners without this.

Because the trace source Listeners needs to be accessible to enable programmatically adding listeners. See examples in public doc: https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/how-to-create-and-initialize-trace-sources#to-initialize-trace-sources-listeners-and-filters-without-a-configuration-file

            mySource.Listeners.Add(console);
            mySource.Listeners.Add(textListener);

[daviburg]

I also don't like these "magic" event ids. Perhaps consider using an internal enum for this?

Yes "magic" event ids are not ideal. I agree that an enum is the best way to address that. I will add an enum for trace event ids. It will also become handy if we latter either upgrade or supplement trace source with a modern ETW event source (they also use unique id per event).

[daviburg]

@drieseng could you take a new look at this?

[daviburg]

@drieseng ping for review / next step

[daviburg]

@drieseng I noticed you are active today. Could you review the answers here?

[drieseng]

• You cannot use string interpolation as we still support target framework that require us to use an old(er) C# compiler.

SSH .NET targets Framework 3.5 as the oldest, and all framework version are compatible with C# version 7.3 per https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version#defaults

I have to use VS 2012 to build some of our older target frameworks (Silverlight, WP7). Please remove usage of string interpolation.

[drieseng]

I left some remarks in my latest review. The most important remarks are:

1. Revert usage of string interpolation in order to keep support for older target frameworks.
2. Do not make DiagnosticAbstraction public.
3. Keep the Log(...) method conditional.

I know these last two remarks are problematic, but I'd like to discuss and review these in more detail. Note that I definitely agree that a user should not have to rebuild SSH.NET to enable logging.

[daviburg]

I left some remarks in my latest review. The most important remarks are:

1. Revert usage of string interpolation in order to keep support for older target frameworks.
2. Do not make DiagnosticAbstraction public.
3. Keep the Log(...) method conditional.

I know these last two remarks are problematic, but I'd like to discuss and review these in more detail. Note that I definitely agree that a user should not have to rebuild SSH.NET to enable logging.

Ok, applied all 3 of these asks.

For modernization, I recommend replacing this legacy trace with System.Diagnostics.Tracing.EventSource. We would need a SshNetEventSource class derived from EventSource with an event source attribute (e.g. [EventSource(Name = "SSH.NET")]) and a method per event type. The new event ids introduced in this PR should be possible to re-use as-is.

The per event type methods would look something like:

        [Event(eventId: (int)AdapterEventIds.ListenerAcceptChannelClosedEarly, Level = EventLevel.Warning)]
        public void ListenerAcceptChannelClosedEarly(string listenerUri, string message)
        {
            this.WriteEvent(eventId: (int)AdapterEventIds.ListenerAcceptChannelClosedEarly, arg1: listenerUri, arg2: message);
        }

For convenience I typically add a lazy-initialized singleton like that:

        private static readonly Lazy<SshNetEventSource> EventSourceInstance = new Lazy<SshNetEventSource>(() => new SshNetEventSource());

        public static SshNetEventSource Log => EventSourceInstance.Value;

        /// <summary>
        /// Prevents construction of instances by others, enforcing singleton pattern as intended by .NET team authors of EventSource
        /// </summary>
        private SshNetEventSource()
        {
        }

There is an EventSource User's Guide by Cosmin Radu and Vance Morrison, architects from the .NET team, on this topic.

[WojciechNagorski]

@daviburg Can you refresh this PR? We've already removed old .NET Frameworks.