Let’s do DHCP: more diagnostic events

While thinking about how to combine latency measurement with diagnostic events in the DHCP server, I noticed a particular gap in the design. Right now the event interfaces look like this:

public interface ISocketEvents
{
    public void SendStart(SocketId id, int bufferSize, IPEndpointV4 endpoint)
    {
        // . . .
    }
 
    public void SendEnd(SocketId id, bool succeeded, Exception exception)
    {
        // . . .
    }
 
    // . . . etc.
}

However, if we want to incorporate latency metrics, we would need to somehow initialize a TimePoint in SendStart and use it within SendEnd to calculate the total duration. There are various fancy tricks we could employ in making this work but they would be just that — fancy tricks. Where possible, we should dispense with the magic and try just passing parameters. What might that look like in this case?

We can express the requirement as follows: we need the ability to create something in XyzStart and use that something in XyzEnd. It seems that the easiest way to proceed would be defining a state parameter:

public interface ISocketEvents<TState>
{
    public TState SendStart(SocketId id, int bufferSize, IPEndpointV4 endpoint)
    {
        // . . . return a TState instance here
    }
 
    public void SendEnd(SocketId id, bool succeeded, Exception exception, TState state)
    {
        // . . . use the TState instance here
    }
 
    // . . . etc.
}

Now, tracking latency would be trivial:

public class SocketEventsWithLatency : ISocketEvents<TimePoint>
{
    public TimePoint SendStart(SocketId id, int bufferSize, IPEndpointV4 endpoint)
    {
        TimePoint start = TimePoint.Now();
        // . . .
        return start;
    }
 
    public void SendEnd(SocketId id, bool succeeded, Exception exception, TimePoint state)
    {
        // . . .
        TimeSpan latency = TimePoint.Now() - state;
        // send the latency value to some reporting system . . .
    }
 
    // . . . etc.
}

This design is simple and flexible. Perhaps best of all, it is optional. If you can get by without a state parameter, then you can use the original non-generic interfaces.

There is still one more thing bothering me about this design. Let’s take a look at the SendEnd method:

    public void SendEnd(SocketId id, bool succeeded, Exception exception, TState state)
    {
        // . . .
    }

It might be hard to see the problem. But pay attention to the bool/Exception parameters. These are related items but they are passed separately — not just here, but in every XyzEnd method. We have a data clump which is begging to be extracted into a parameter object. In this case, we could make a simple struct with nicely readable named constructors:

    public readonly struct OperationStatus
    {
        private OperationStatus(bool succeeded, Exception exception)
        {
            this.Succeeded = succeeded;
            this.Exception = exception;
        }

        public bool Succeeded { get; }

        public Exception Exception { get; }

        public static OperationStatus Success() =>
            new OperationStatus(true, null);

        public static OperationStatus Failure(Exception exception) =>
            new OperationStatus(false, exception);
    }

It doesn’t look like much but even this small change does a lot to reveal the intention of the calling code at a glance:

            public async Task SendAsync(ReadOnlyMemory<byte> buffer, IPEndpointV4 endpoint)
            {
                Guid activityId = ActivityScope.CurrentId;
                TState state = this.socketEvents.SendStart(this.id, buffer.Length, endpoint);
                try
                {
                    await this.inner.SendAsync(buffer, endpoint);
                    this.OnEndSend(activityId, OperationStatus.Success(), state);
                }
                catch (Exception e)
                {
                    this.OnEndSend(activityId, OperationStatus.Failure(e), state);
                    throw;
                }
            }

For reference, this is what it looked like before:

            public async Task SendAsync(ReadOnlyMemory<byte> buffer, IPEndpointV4 endpoint)
            {
                Guid activityId = ActivityScope.CurrentId;
                TState state = this.socketEvents.SendStart(this.id, buffer.Length, endpoint);
                try
                {
                    await this.inner.SendAsync(buffer, endpoint);
                    this.OnEndSend(activityId, null, state);
                }
                catch (Exception e)
                {
                    this.OnEndSend(activityId, e, state);
                    throw;
                }
            }

The structure is the same and it is still readable, but the success and failure conditions are somewhat more hidden. If nothing else, a pattern that explicitly uses the literal terms “Success” and “Failure” in its public interface ought to be harder to misuse.

If you want the full details of this design and refactoring journey, check out the DhcpServer commits on GitHub.