[API Proposal]: support System.Diagnostics.TraceSource to be initialized from the app.config file.

[steveharter]

Background and motivation

Original issue: https://github.com/dotnet/runtime/issues/23937

This helps adoption from those moving from .NET Framework who use TraceSource for logging. There is currently no support for automatically reading the app.config and applying those settings to TraceSource and associated TraceFilter and TraceSwitch instances. Support for Trace.Refresh() based on .NET Framework is also added which allows the application to update existing TraceSource and Switch instances when the config file changes.

The TraceSource code lives in System.Diagnostics.TraceSource.dll which is shipped inbox but the processing of the app.config is done in System.Configuration.ConfigurationManager.dll which is OOB.

The proposed APIs essentially allow interaction between these two separate assemblies and these APIs are used internally and not intended for general use. The one exception is the TraceConfiguration.Register() method to light-up the config side.

Although not a stated goal in the issue above, these new APIs make it possible to support reading from another format, such as a JSON file, by using a similar approach as done in System.Configuration.ConfigurationManager.dll which reads from the app.config XML file.

API Proposal

These live in System.Diagnostics.TraceSource.dll:

namespace System.Diagnostics
{
    public abstract class Switch
    {
         // Needed to reset Value if this Switch is removed from config and Trace.Refresh is called.
+        public string DefaultValue { get { throw null; } }

-        protected string Value { get; set; }
+        public string Value { get; set; }

         // Called by the config system when Trace.Refresh() is called.
+        public void Refresh();

         // Invoke the config system to initialize the switch from the config file.
+        public static event EventHandler<InitializingSwitchEventArgs>? Initializing { add; remove; }
    }

    public sealed class Trace
    {
         // Invoke the config system to refresh TraceSources from the config file
+        public static event EventHandler? Refreshing{ add; remove; }
    }

    public class TraceSource
    {
         // Needed to reset Level if removed from config and Trace.Refresh is called.
+        public SourceLevels DefaultLevel { get; }

         // Invoke the config system to initialize a TraceSource.
+        public static event EventHandler<InitializingTraceSourceEventArgs>? Initializing { add; remove; }
    }

+    public sealed class InitializingSwitchEventArgs : EventArgs
+    {
+        public InitializingSwitchEventArgs(Switch @switch);
+        public System.Diagnostics.Switch Switch { get; }
+    }

+    public sealed class InitializingTraceSourceEventArgs : EventArgs
+    {
+        public InitializingTraceSourceEventArgs(TraceSource traceSource) { }
+        public System.Diagnostics.TraceSource TraceSource { get; }
+        public bool WasInitialized { get; set; }
+    }
}

These live in System.Configuration.ConfigurationManager.dll:

// Use the Diagnostics namespace even though in config
namespace System.Diagnostics
{
+    public static class TraceConfiguration
+    {
+        // Subscribe to the new events and create TraceSources etc. from the config file.
+        // This is really the only new API that is called by the user; the rest are for internal use.
+        // Is there a better name here? Such as "EnableConfigurationFiles()"
+        public static void Register();
+    }
}

API Usage

Given the app.config file:

<configuration>
  <system.diagnostics>
    <sources>
      <source name="TestTraceSource">
        <listeners>
          <add name = "listener" type="System.Diagnostics.TextWriterTraceListener" initializeData="listener.log" />
        </listeners>
      </source>
    </sources>
  </system.diagnostics>
</configuration>

Then code using TraceSource will be initialized to the values in the app.config:

TraceConfiguration.Register(); // Needed to light up the config system (not necessary in .NET Framework)
TraceSource trace = new("TestTraceSource");
int i = trace.Listeners.Count; // Returns 2; one for the default, and one for the entry in the app.config

Alternative Designs

No response

Risks

No response

[ghost]

Tagging subscribers to this area: @tommcdon See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation Original issue: https://github.com/dotnet/runtime/issues/23937 This helps adoption from those moving from .NET Framework which use `TraceSource` for logging. There is currently no support for automatically reading the `app.config` and from that creating the various `TraceSource` and associated `TraceFilter` and `TraceSwitch` entries. The `TraceSource` code lives in `System.Diagnostics.TraceSource.dll` which is shipped inbox but the processing of the `app.config` is done in `System.Configuration.ConfigurationManager.dll` which is OOB. The proposed APIs essentially allow interaction between these two separate assemblies and these APIs are used internally and not intended for general use. The one exception is the `TraceConfiguration.Register()` method to light-up the config side. Although not a stated goal in the issue above, these new APIs make it possible to support reading from another format, such as a ` json` file, by using a similar approach as done in `System.Configuration.ConfigurationManager.dll` which reads from the `app.config` XML file. ### API Proposal These live in System.Diagnostics.TraceSource.dll: ```diff namespace System.Diagnostics { public abstract class Switch { - public System.Collections.Specialized.StringDictionary Attributes { get; } + public System.Collections.Specialized.StringDictionary Attributes { get; set; } + public string DefaultValue { get { throw null; } } - protected string Value { get; set; } + public string Value { get; set; } + public void Refresh(); } public sealed class Trace { + public static event EventHandler? ConfigureSwitch { add; remove; } + public static event EventHandler? ConfigureTrace { add; remove; } + public static event EventHandler? ConfigureTraceSource { add; remove; } } public abstract class TraceListener { - public System.Collections.Specialized.StringDictionary Attributes { get; } + public System.Collections.Specialized.StringDictionary Attributes { get; set; } } public class TraceSource { - public System.Collections.Specialized.StringDictionary Attributes { get; } + public System.Collections.Specialized.StringDictionary Attributes { get; set; } + public SourceLevels DefaultLevel { get; } } ``` These live in System.Configuration.ConfigurationManager.dll: ```diff // Use the Diagnostics namespace even though in config namespace System.Diagnostics { + public static class TraceConfiguration + { + // Subscribe to the new events and create TraceSources etc. from the config file. + // This is really the only new API that is called by the user; the rest are for internal use. + public static void Register(); + } } ``` ### API Usage Given the `app.config` file: ``` ``` Then code using `TraceSource` will be initialized to the values in the `app.config`: ```csharp TraceConfiguration.Register(); // Needed to light up the config system (not necessary in .NET Framework) TraceSource trace = new("TestTraceSource"); int i = trace.Listeners.Count; // Returns 2; one for the default, and one for the entry in the app.config ``` ### Alternative Designs _No response_ ### Risks _No response_

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Diagnostics`, `api-ready-for-review`
Milestone:	7.0.0

[noahfalk]

Naming nit: the verb 'Register' wasn't intuitive for me in this context. As an application author I'd be more inclined to think of it as Apply()ing or Load()ing the Trace configuration.

I imagine Register makes sense in the context of registering for configuration callbacks from System.Diagnostics.TraceSource, but if the application author isn't aware of what the new TraceSource API looks like or how the Register() function is implemented then the meaning may not land for them.

[steveharter]

Naming nit: the verb 'Register' wasn't intuitive for me in this context.

Hmm:

• "TraceConfiguration.EnableConfigurationFiles()"
• "TraceConfiguration.EnableAppConfig()"
• "TraceConfiguration.EnableAppConfigLoading()"

[noahfalk]

IMO all of those are an improvement over Register, EnableAppConfig() sounds the best to me out of the group but I know naming is a very subjective topic : ) Thanks!

[terrajobst]

• If the goal is to have parity with .NET Framework then we should probably not have more APIs than .NET Framework
  • It seems we need to make some changes because in .NET Framework each subsystem that supported configuration would call into configuration to read data from app.config which we don't do in .NET Core because we treat app.config as legacy API that sits on top of everything else.
• This looks this needs another look to see whether we can minimize the API surface
  • By moving the events to the corresponding types we might be able to get rid of the EventArgs and just require handlers to cast sender
  • Why do we need to expose the defaults? This wasn't necessary on .NET Framework.

Could we make this API shape work?

namespace System.Diagnostics
{
    public abstract class Switch
    {
-        protected string Value { get; set; }
+        public string Value { get; set; }
+        public void Refresh();
+        public static event EventHandler? Initializing { add; remove; }
    }

    public sealed class Trace
    {
+        public static event EventHandler? Refreshing { add; remove; }
    }

    public class TraceSource
    {
+        public SourceLevels DefaultLevel { get; }
+        public static event EventHandler<ConfigureTraceSourceEventArgs>? Initializing { add; remove; }
    }

+    public sealed class ConfigureTraceSourceEventArgs : EventArgs
+    {
+        public ConfigureTraceSourceEventArgs();
+        public bool WasConfigured { get; set; }
+    }
}

[steveharter]

By moving the events to the corresponding types we might be able to get rid of the EventArgs and just require handlers to cast sender

Since these are static events, the guidelines state: _DO pass null as the sender when raising a static event.

This implies bringing back ConfigureSwitchEventArgs and ConfigureTraceSourceEventArgs.TraceSource.

Why do we need to expose the defaults? This wasn't necessary on .NET Framework.

These are needed in case a TraceSource or Switch is removed from a config file and Trace.Refresh() is called which re-reads from the config file. The .NET Framework behavior is to reset these back to the default specified in the TraceSource(...) and Switch(...)` constructors. The .NET Framework reference source: TraceSource and Switch.

As for removing the setters for public System.Collections.Specialized.StringDictionary Attributes, we can avoid adding a setter although it is not as performant since a Clear() followed by manual copy from the underlying config's internal collection:

        internal static void CopyStringDictionary(StringDictionary source, StringDictionary dest)
        {
            dest.Clear();
            foreach (string key in source)
            {
                dest[key] = source[key];
            }
        }

On naming, "InitializingTraceSourceEventArgs" seems more appropriate than "ConfigureTraceSourceEventArgs" and "WasInitialized" instead of "WasConfigured".

[terrajobst]

We did a review update via email and agreed changed the API shape to this:

namespace System.Diagnostics
{
    public abstract class Switch
    {
+        public string DefaultValue { get { throw null; } }

-        protected string Value { get; set; }
+        public string Value { get; set; }

+        public void Refresh();
+        public static event EventHandler<InitializingSwitchEventArgs>? Initializing { add; remove; }
    }

    public sealed class Trace
    {
+        public static event EventHandler? Refreshing { add; remove; }
    }

    public class TraceSource
    {
+        public SourceLevels DefaultLevel { get; }
+        public static event EventHandler<InitializingTraceSourceEventArgs>? Initializing { add; remove; }
    }

+    public sealed class InitializingSwitchEventArgs : EventArgs
+    {
+        public InitializingSwitchEventArgs(Switch @switch);
+        public System.Diagnostics.Switch Switch { get; }
+    }

+    public sealed class InitializingTraceSourceEventArgs : EventArgs
+    {
+        public InitializingTraceSourceEventArgs(TraceSource traceSource) { }
+        public System.Diagnostics.TraceSource TraceSource { get; }
+        public bool WasInitialized { get; set; }
+    }

+    public static class TraceConfiguration
+    {
+        public static void Register();
+    }
}

Reasons given by @steveharter:

• Switch.DefaultValue and TraceSource.DefaulLevel are required for reasons mentioned here.
• Static events should pass null for sender, so adding back InitializingSwitchEventArgs and InitializingTraceSourceEventArgs.TraceSource. If we want to go against the guidelines here, I'm open to that.
• Rename InitializingTraceSourceEventArgs.WasConfigured to WasInitialized.
• Rename ConfigureTraceSourceEventArgs to InitializingTraceSourceEventArgs
• The approved API did not include TraceConfiguration.Register(). Note that we did not discuss naming of this and are some community comments about it; essentially it tells the config system to subscribe and handle the 3 events.
  • Note that this API lives in the config assembly, not the diagnostics assembly.

[RussKie]

If we're using "initializing" may be the method should as well be called Initialize() as opposed to Register()?

I may be wrong, but I think the main ask came from Windows Forms users, and those users would likely be the main audience for this API. Windows Forms SDK has another API that is run at the start of the app - the same place where this API will be run as well - which is called AppConfiguration.Initialize(). Aligning the vocabulary would be great.