DiagnosticSource questions for IdentityServer

[leastprivilege]

Hey Mark,

I am currently researching if this could be the right approach for IdentityServer vNext...

I have a couple of questions

• Do I always have to new up the DS myself? and then pass the instance around over multiple method calls
• Especially when using the activity feature and that activity e.g. spans multiple methods in different classes
• Is there a way to combine that with DI? How do the aspnetcore folks use this?

thanks!

[markrendle]

I definitely think it could be the right approach, the more frameworks/libraries that add support for DS, the better.

I've used various techniques, including: a static instance per class for application code; a shared static instance for a project; and a wrapping class for DI.

You can't just register your own DiagnosticSource with Extensions.DependencyInjection because AspNetCore registers its own "Microsoft.AspNetCore" singleton. The EF Core folks do something weird that I can't work out, but they've combined Logging and Diagnostics together.

For a package like yours with multiple components, I'd suggest creating a wrapper class around your own instance, something like this:

    internal interface IIdentityServerDiagnostics
    {
        bool IsEnabled(string name);
        bool IsEnabled(string name, object arg1, object arg2 = null);
        void Write(string name, object value);
        Activity StartActivity(Activity activity, object args);
        void StopActivity(Activity activity, object args);
    }

    internal sealed class IdentityServerDiagnostics : IIdentityServerDiagnostics
    {
        private readonly DiagnosticSource _source = new DiagnosticListener("IdentityServer4");
        public bool IsEnabled(string name) => _source.IsEnabled(name);
        public bool IsEnabled(string name, object arg1, object arg2 = null) => _source.IsEnabled(name, arg1, arg2);
        public void Write(string name, object value) => _source.Write(name, value);
        public Activity StartActivity(Activity activity, object args) => _source.StartActivity(activity, args);
        public void StopActivity(Activity activity, object args) => _source.StopActivity(activity, args);
    }

Then you can register that as a singleton with DI, and also mock it for your tests.

[leastprivilege]

Thanks Mark! I will give it a try and see how it feels.

[benaadams]

Go with the type as DiagnosticListener rather than DiagnosticSource as it has a faster isEnabled check (checks if anything is attached) which you can do prior to the slower with params. As usually nothing is listening

internal sealed class IdentityServerDiagnostics : IIdentityServerDiagnostics
{
-   private readonly DiagnosticSource _source = new DiagnosticListener("IdentityServer4");
+   private readonly DiagnosticListener _listener = new DiagnosticListener("IdentityServer4");
-   public bool IsEnabled(string name) => _source.IsEnabled(name);
+   public bool IsEnabled(string name) => _listener.IsEnabled() && _listener.IsEnabled(name);
-   public bool IsEnabled(string name, object arg1, object arg2 = null) => _source.IsEnabled(name, arg1, arg2);
    public void Write(string name, object value) => _source.Write(name, value);
    public Activity StartActivity(Activity activity, object args) => _source.StartActivity(activity, args);
    public void StopActivity(Activity activity, object args) => _source.StopActivity(activity, args);
}

For events themselves; mvc recently moved to strongly typed dotnet/aspnetcore#11730 public events to make it easier for consumers with the following pattern

Base class helper

namespace IdentityServer.Diagnostics
{
    public abstract class EventData : IReadOnlyList<KeyValuePair<string, object>>
    {
        protected const string EventNamespace = "IdentityServer4.";

        protected abstract int Count { get; }
        protected abstract KeyValuePair<string, object> this[int index] { get; }

        int IReadOnlyCollection<KeyValuePair<string, object>>.Count => Count;
        KeyValuePair<string, object> IReadOnlyList<KeyValuePair<string, object>>.this[int index] => this[index];

        Enumerator GetEnumerator() => new Enumerator(this);

        IEnumerator<KeyValuePair<string, object>> IEnumerable<KeyValuePair<string, object>>.GetEnumerator()
            => GetEnumerator();

        IEnumerator IEnumerable.GetEnumerator()
            => GetEnumerator();

        public struct Enumerator : IEnumerator<KeyValuePair<string, object>>
        {
            private readonly EventData _eventData;
            private readonly int _count;

            private int _index;

            public KeyValuePair<string, object> Current { get; private set; }

            internal Enumerator(EventData eventData)
            {
                _eventData = eventData;
                _count = eventData.Count;
                _index = -1;
                Current = default;
            }

            public bool MoveNext()
            {
                var index = _index + 1;
                if (index >= _count)
                {
                    return false;
                }

                _index = index;

                Current = _eventData[index];
                return true;
            }

            public void Dispose() { }
            object IEnumerator.Current => Current;
            void IEnumerator.Reset() => throw new NotSupportedException();
        }
    }
}

An event (properties and name are up to you)

namespace IdentityServer.Diagnostics
{
    public sealed class AfterAction : EventData
    {
        public const string EventName = EventNamespace + nameof(AfterAction);

        public AfterAction(ActionDescriptor actionDescriptor, HttpContext httpContext, RouteData routeData)
        {
            ActionDescriptor = actionDescriptor;
            HttpContext = httpContext;
            RouteData = routeData;
        }

        public ActionDescriptor ActionDescriptor { get; }
        public HttpContext HttpContext { get; }
        public RouteData RouteData { get; }

        protected override int Count => 3;

        protected override KeyValuePair<string, object> this[int index] => index switch
        {
            0 => new KeyValuePair<string, object>(nameof(ActionDescriptor), ActionDescriptor),
            1 => new KeyValuePair<string, object>(nameof(HttpContext), HttpContext),
            2 => new KeyValuePair<string, object>(nameof(RouteData), RouteData),
            _ => throw new IndexOutOfRangeException(nameof(index))
        };
    }
}

Internal helper to emit those events

namespace IdentityServer.Diagnostics
{
    internal static class IdentityServerDiagnosticsExtensions
    {
        public static void AfterAction(
            this IIdentityServerDiagnostics listener,
            ActionDescriptor actionDescriptor,
            HttpContext httpContext,
            RouteData routeData)
        {
            if (listener.IsEnabled())
            {
                AfterActionImpl(listener, actionDescriptor, httpContext, routeData);
            }
        }

        private static void AfterActionImpl(IIdentityServerDiagnostics listener, ActionDescriptor actionDescriptor, HttpContext httpContext, RouteData routeData)
        {
            if (listener.IsEnabled(AfterAction.EventName))
            {
                listener.Write(
                    AfterAction.EventName,
                    new AfterAction(actionDescriptor, httpContext, routeData));
            }
        }
    }
}

So usage in Identity server code to emit the above event if a listener is attached and listening to that event would be:

diagnostics.AfterAction(actionDescriptor, httpContext, routeData);

Assuming you injected IIdentityServerDiagnostics as diagnostics

[leastprivilege]

Wow - thanks @benaadams ! OK - I need to give this a try next week.

[leastprivilege]

@benaadams What's the motivation for the base-class and the list over the event data?

[leastprivilege]

nevermind - saw your post

Type implements IReadOnlyList<KeyValuePair<string, object>> so the data can be iterated over.

Type implements IEnumerable<KeyValuePair<string, object>> so the data can be enumerated over,

[benaadams]

And a different type for each event type; so people can use pattern matching if they like that kinda thing.

The base-class is just to provide a nice range of options for consumption depending on want the event consumer want to use; while not making it onerous to implement.

[leastprivilege]

We were def. planning on using a different type for each event because we have the challenge that some people need to redact certain event data - so easy filtering is required.