8
\$\begingroup\$

When testing or debugging applications it'd be often useful to be able to run it without certain features like committing things to the database, saving files, sending emails, reading files, executing queries and and and... Currently I use various configurations and boolean flags to achieve this goal.

This is a very tedious and repetitive practice, so I was looking for something easier to use. My first thought was Dependency Injection but I quickly realised that I'll never even get close to the same result as there can by anything that could be named feature.

Then I came up with the idea of a FeatureService that is a class that allows me to configure each feature independently. It encapsulates the pattern that I usually use for toggling features. By default all features are enabled.

It is more than just a simple on/off mechanism that executs some Func. It also provides logging of additional information about features like a warning when it's toggled from its default state, or the elapsed time, or an alternative result in case a feature needs to return some value.


goto:(Toggle any application feature ON or OFF - follow-up (v2))


public class FeatureService
{
    private readonly FeatureOptions _defaultOptions;
    private readonly ILogger _logger;
    private readonly IDictionary<string, FeatureOptions> _options = new Dictionary<string, FeatureOptions>();

    public FeatureService(ILogger<FeatureService> logger, FeatureOptions defaultOptions = Enabled | Warn | Telemetry)
    {
        _logger = logger;
        _defaultOptions = defaultOptions;
    }

    public async Task<T> ExecuteAsync<T>(string name, Func<Task<T>> body, Func<Task<T>> bodyWhenDisabled)
    {
        var options =
            _options.TryGetValue(name, out var customOptions)
                ? customOptions
                : _defaultOptions;

        using (_logger.BeginScope().WithCorrelationHandle("Feature").AttachElapsed())
        {
            if (options.HasFlag(Enabled))
            {
                if (options.HasFlag(Warn) && !_defaultOptions.HasFlag(Enabled))
                {
                    _logger.Log(Abstraction.Layer.Service().Decision($"Using feature '{name}'").Because("Enabled").Warning());
                }
            }
            else
            {
                body = bodyWhenDisabled;

                if (options.HasFlag(Warn) && _defaultOptions.HasFlag(Enabled))
                {
                    _logger.Log(Abstraction.Layer.Service().Decision($"Not using feature '{name}'").Because("Disabled").Warning());
                }
            }

            // Not catching exceptions because the caller should handle them.
            try
            {
                return await body();
            }
            finally
            {
                _logger.Log(Abstraction.Layer.Service().Routine(name).Completed());
            }
        }
    }

    public FeatureService Configure(string name, Func<FeatureOptions, FeatureOptions> configure)
    {
        _options[name] =
            _options.TryGetValue(name, out var options)
                ? configure(options)
                : configure(_defaultOptions);

        return this;
    }
}

[Flags]
public enum FeatureOptions
{
    None = 0,

    /// <summary>
    /// When set a feature is enabled.
    /// </summary>
    Enabled = 1 << 0,

    /// <summary>
    /// When set a warning is logged when a feature is toggled.
    /// </summary>
    Warn = 1 << 1,

    /// <summary>
    /// When set feature usage statistics are logged.
    /// </summary>
    Telemetry = 1 << 2, // For future use
}

This is the core API and the only implementation. Since it'd be difficult to use it in this form in many situations, all other convenience APIs are (going to be) extensions, e.g.:

public static class FeatureServiceExtensions
{
    public static void Execute(this FeatureService features, string name, Action body, Action bodyWhenDisabled)
    {
        features.ExecuteAsync(name, () =>
        {
            body();
            return Task.FromResult(default(object));
        }, () =>
        {
            bodyWhenDisabled();
            return Task.FromResult(default(object));
        }).GetAwaiter().GetResult();
    }
}

These extensions are not limited only to the ExecuteAsync API. The Configure API can also be extented by various extensions like Enable or Disable that are encapsulating the enum logic.

I also have other tools that let me avoid actually using strings here but I'll post them another time in a new question.

This means that you should treat this proof-of-concept as the deepest core service that needs a couple of additional extension methods to make its usage more convenient (e.g. toggling all database features at once etc.) I also have another PoC for that but one step at a time...

Demo

Here's an example of how it I mean to use it. Most applications will receive the instance via DI.

void Main()
{
    var demo = new FeatureServiceDemo();
    demo.Start();
}

public class FeatureServiceDemo
{
    private readonly FeatureService _features = new FeatureService(Logger<FeatureService>.Null);

    public void Start()
    {
        SayHallo();

        _features.Configure(nameof(SayHallo), o => o ^ Enabled);

        SayHallo();
    }

    private void SayHallo()
    {
        _features.Execute
        (
            nameof(SayHallo), 
            () => Console.WriteLine("Hallo"), 
            () => Console.WriteLine("You've disabled it!")
        );
    }
}

If someone wanted to play with it, here's the LINQPad's header:

<Query Kind="Program">
  <NuGetReference>Reusable.Core</NuGetReference>
  <NuGetReference>Reusable.OmniLog.SemanticExtensions</NuGetReference>
  <Namespace>Reusable.Data</Namespace>
  <Namespace>Reusable.Extensions</Namespace>
  <Namespace>Reusable.OmniLog</Namespace>
  <Namespace>Reusable.OmniLog.Abstractions</Namespace>
  <Namespace>Reusable.OmniLog.SemanticExtensions</Namespace>
  <Namespace>static FeatureOptions</Namespace>
  <Namespace>System.Linq</Namespace>
  <Namespace>System.Linq.Custom</Namespace>
  <Namespace>System.Threading.Tasks</Namespace>
</Query>

Questions

  • Do you think this service is useful?
  • How would you improve it?
  • What scenarios do you think I couldn't handle with it?
  • I'm currently building it mainly for console applications and for one-time setup but I believe it'll quickly get reused in other types of applications so I'm already thinking about what happens when I decide to toggle a feature at runtime... I guess, I'll need to switch to the ConcurrentDictionary then.
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm eagerly awaiting answers to this question, since we are currently facing the same problem as you. Instant +1 from me. I have 1 remark, but it's not enough to make an answer, so I will say it here. [Flags] enum are best used as uint. \$\endgroup\$ – dfhwze May 24 at 18:28
  • 1
    \$\begingroup\$ @t3chb0t I've recently read an interesting article published at Martin Fowlers Blog regarding how to design such things, and what are the exact dimensions to take into consideration: martinfowler.com/articles/feature-toggles.html \$\endgroup\$ – πάντα ῥεῖ May 24 at 18:46
  • 1
    \$\begingroup\$ @πάνταῥεῖ done reading! It turns out that I've implemented it exactly as it is described there and I can also easily adjust it so that it supports other types of features like release/experimental/etc. Nice, mr Fowler can learn from me something too, he's not the only software genius ;-P \$\endgroup\$ – t3chb0t May 26 at 9:40
  • 1
    \$\begingroup\$ This reminds me a lot of the Microsoft.FeatureManagement package on Nuget \$\endgroup\$ – Nick Albrecht Jul 25 at 0:45
  • 1
    \$\begingroup\$ The Microsoft.FeatureManagement.AspNetCore one is, yes. But I believe Microsoft.FeatureManagement can be used in non ASP.NET Core projects. Though it does use some other aspects common to them, like logging, DI, and Configuration. Still, interesting :-) I heard about it from here. youtube.com/watch?v=cmd0Vh6pFfE \$\endgroup\$ – Nick Albrecht Jul 25 at 19:44
4
\$\begingroup\$

Strings as tokens

I'm assuming the following comment is anticipating this other question, where strings remain part of the API, but some of my complaints with them are addressed.

I also have other tools that let me avoid actually using strings here but I'll post them another time in a new question.

Personally I don't like using strings for this sort of thing: it's not expressive and static enough for me. I'd much rather there was a static readonly FeatureKey SayHalloFeature somewhere, which was the singular reference for everything. If you don't want statics all over the place, you can cover classes nicely with generics, but not methods (as per your other question); though my opinion is that method names have no meaning outside of their class/interface, and if you want SayHallo to be disablable, you should document the fact clearly by providing a FeatureKey (or whatever) that will show up in the intelli-sense for a consumer (improving discoverability), rather than their having to check the written documentation (which hopefully existing, but may not be updated for this exact method) for each method they might want to disable, and forever looking over their shoulder incase they create a collision.

Other benefits of using explicit tokens over strings:

  • You can limit access to tokens, so that not anyone can disable "OSSpecificCriticalSecurityService" by accident
  • It's harder to produce binary-breaking changes (things like const string can go wrong)
  • Tokens give you extensibility options:
    • You could set defaults in the Token (these defaults could be a function of whether it is a Debug/Release build, etc.)
    • Include additional meta-deta (long names, logging info etc.) and policy options
  • Easier to manage globalisation (printable strings become an independent concern)

The main downside as I see it is that you can't trivially configure it from a flat file or other text-only source; but at the same time, it gives you better control of what can be configured some elsewhere, and if you want to hunt down suitable annotated tokens with reflection or T4 or something, that remains an option. You can always add a StringyToken class later, if you want to use strings as well (e.g. for things that should be user configurable). The layer of complexity translating flat-file to tokens forces you to ensure the token exists, so you can provide useful messages to users who typo the feature names, instead of enabling "ReliableNetwok" and finding you are still dropping packets.

Other API Comments

As you say, the Configure interface could be extended, and personally I would consider this a priority in order to hide-away any bit-banging: Set(FeatureOptions) and Clear(FeatureOptions) methods would be enough to keep me happy. I'm not sure that facilitating 'toggling' makes sense as part of the basic API at all.

It's notable, that there is no way to query the current FeatureOptions directly. If someone wanted to do this, they might be tempted to abuse Configure, and in doing so by-pass the logging and warning features. An IReadonlyFeatureService type interface might be appropriate, to force the use of the Exectue method(s).

Other

Everything else looks pretty great. The warning-checks are a bit confusing, but I can't think how to improve them.

I might be tempted to cache a Task.FromResult(default(object)); if the Execute(,,Action,Action) extension will be heavily used.

\$\endgroup\$
  • \$\begingroup\$ there is no way to query the current FeatureOptions directly - and there shouldn't be any. I cannot think of any use-case where it would be of interest to the user. If he could do this, then he might as well throw away that service ;-] You are right about the other things. I really use that key generators everywhere now ;-P I like not having const strings because you can very easily change naming conventions and keep everything automtic. \$\endgroup\$ – t3chb0t Jul 24 at 17:49
  • \$\begingroup\$ You can limit access to tokens, so that not anyone can disable "OSSpecificCriticalSecurityService" by accident - I like this one! I definitely need to implement it so that certain features cannot be disabled in production code. About the question why there is no configuration from files etc, I leave that part to other frameworks that can handle this better. \$\endgroup\$ – t3chb0t Jul 24 at 17:51
  • 2
    \$\begingroup\$ @t3chb0t "and there shouldn't be any": this is why I suggest the 'read only' version, so that you can enforce this: at the moment Configure is a weakness, because it can be (ab)used to perform such a query ;) \$\endgroup\$ – VisualMelon Jul 24 at 17:55
  • \$\begingroup\$ Oh, now the Set and Clear make sense. However, I think this should be extensions to the feature-service using Configure internally. If the user wants to do some magic, that is usally me ;-P he should be able to; it shouldn't be something to do everywhere and all the time... this method is very useful for batch modifications where I toggle features based on tags (like disable all database related features) but this is something that the other question about key generators covers. \$\endgroup\$ – t3chb0t Jul 24 at 18:06
  • 1
    \$\begingroup\$ @t3chb0t feel free to self answer :) All ideas are welcome. \$\endgroup\$ – dfhwze Jul 24 at 18:36
5
+50
\$\begingroup\$

Do you think this service is useful?

I don't think it's possible to answer that question with the toy example given. IMO it doesn't shed any light on how you intend to use it for things like

committing things to the database, saving files, sending emails, reading files, executing queries and and and...


    public FeatureService(ILogger<FeatureService> logger, FeatureOptions defaultOptions = Enabled | Warn | Telemetry)

I realise that the limitations of C#'s type system don't allow a good solution here, but ideally the logger would be typed (using Java's covariant notation) as ILogger<? extends FeatureService>. Have you considered using F-bounded polymorphism (FeatureService<T extends FeatureService<T>>)?


    public async Task<T> ExecuteAsync<T>(string name, Func<Task<T>> body, Func<Task<T>> bodyWhenDisabled)

name of what? The body? I think it's actually the label of the toggle, but I'm having to reverse engineer the code to figure that out, rather than relying on the documentation and signature.


            if (options.HasFlag(Enabled))
            {
                if (options.HasFlag(Warn) && !_defaultOptions.HasFlag(Enabled))
                {
                    _logger.Log(Abstraction.Layer.Service().Decision($"Using feature '{name}'").Because("Enabled").Warning());
                }
            }
            else
            {
                ...
                if (options.HasFlag(Warn) && _defaultOptions.HasFlag(Enabled))
                {
                    _logger.Log(Abstraction.Layer.Service().Decision($"Not using feature '{name}'").Because("Disabled").Warning());
                }
            }
    /// <summary>
    /// When set a warning is logged when a feature is toggled.
    /// </summary>
    Warn = 1 << 1,

The way I interpreted the documentation of the flag is very different to what it actually does. To me, "when a feature is toggled" refers to a trigger/event which is fired by calling Configure.


            try
            {
                return await body();
            }

If this is intended to be a library, which I think it is, it's missing .ConfigureAwait(false). If you don't put it in now, you'll end up debugging a deadlock when you try using this in a WPF application.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you also expand this: ideally the logger would be typed? I'm not sure how you mean that, to me it is typed. I admit, the naming of the toggles is a little bit unclear but I cannot think of anything else right now :-( \$\endgroup\$ – t3chb0t Jul 24 at 17:59
  • 1
    \$\begingroup\$ Either the person that has invented .ConfigureAwait(false) should be fired or the one that has designed async/await in such a way that it is necessary to use it :-\ I read countless blogs about it (the documentation still lacks a good explanation) but I still don't get it. To me it's pure magic. Either use it or suffer for whatever reason :-( \$\endgroup\$ – t3chb0t Jul 25 at 7:15
  • 2
    \$\begingroup\$ @t3chb0t, I agree: the designers got the default wrong. I read blogs.msdn.microsoft.com/benwilli/2017/02/09/… recently, and it's an interesting idea which I may look into using in the future, but I'd want to write a Roslyn analyser to enforce its usage first. Currently I have <Rules AnalyzerId="Microsoft.CodeQuality.Analyzers" RuleNamespace="Microsoft.CodeQuality.Analyzers">...<Rule Id="CA2007" Action="Error" /></Rules> in my ruleset. \$\endgroup\$ – Peter Taylor Jul 25 at 7:20
  • 1
    \$\begingroup\$ The ConfigureAwait comment was crucial for this API to preserve threading integrity. Also, the API could be written better with regards to readability and usability. The bounty is yours. \$\endgroup\$ – dfhwze Jul 28 at 11:02
  • 1
    \$\begingroup\$ @dfhwze & PeterTaylor ... how is that possible that ef-core doesn't use ConfigureAwait(false) anywhere? Apparently there is no single truth about it because they weren't doing it and then they were voting Re-discuss how we handle syncronization context (e.g. ConfigureAwait(false) or similar) I think I won't do it either. It's pure magic... even to experts. \$\endgroup\$ – t3chb0t Jul 28 at 15:18
2
\$\begingroup\$

(self-answer)


Decorators to the recue!

Based on the reviews and also comments I completely redesigned my FeatureToggle. Instead of trying to squeeze everything into the main class I decomposed it to separate classes and their decorators.


The IFeatureToggle interface has lost methods for feature options:

[PublicAPI]
public interface IFeatureToggle
{
    IFeatureOptionRepository Options { get; }

    Task<T> ExecuteAsync<T>(string name, Func<Task<T>> body, Func<Task<T>> fallback);
}

It now has only one property for options and the execute method. I put IsEnabled into an extension:

    public static bool IsEnabled(this IFeatureToggle toggle, string name)
    {
        return toggle.Options.GetOption(name).Contains(FeatureOption.Enable);
    }

The new interface IFeatureOptionRepository is now responsible for keeping feature options.

public interface IFeatureOptionRepository
{
    bool IsDirty(string name);

    [NotNull]
    FeatureOption GetOption(string name);

    [NotNull]
    FeatureOption UpdateOption(string name, Func<FeatureOption, FeatureOption> configure);
}

It has a default implementation with a dictionary that previously was a part of FeatureToggle:

public class FeatureOptionRepository : IFeatureOptionRepository
{
    private readonly IDictionary<string, FeatureOption> _options = new Dictionary<string, FeatureOption>(SoftString.Comparer);

    public bool IsDirty(string name) => _options.ContainsKey(name);

    public FeatureOption GetOption(string name)
    {
        return _options.TryGetValue(name, out var option) ? option : FeatureOption.None;
    }

    public FeatureOption UpdateOption(string name, Func<FeatureOption, FeatureOption> configure)
    {
        var newOption = configure(GetOption(name));
        if (newOption == FeatureOption.None)
        {
            // Don't store default options.
            _options.Remove(name);
        }
        else
        {
            _options[name] = newOption;
        }

        return newOption;
    }
}

You might noticed that there is no default option anymore. I created another decorator to support this:

public class FeatureOptionFallback : IFeatureOptionRepository
{
    private readonly IFeatureOptionRepository _options;
    private readonly FeatureOption _defaultOption;

    public FeatureOptionFallback(IFeatureOptionRepository options, FeatureOption defaultOption)
    {
        _options = options;
        _defaultOption = defaultOption;
    }

    public bool IsDirty(string name) => _options.IsDirty(name);

    public FeatureOption GetOption(string name)
    {
        return _options.GetOption(name) is var option && option != FeatureOption.None ? option : _defaultOption;
    }

    public FeatureOption UpdateOption(string name, Func<FeatureOption, FeatureOption> configure)
    {
        return _options.UpdateOption(name, _ => configure(GetOption(name)));
    }
}

And if I later want to lock some features, I just decorate an option repository with my new FeatureOptionLock:

public class FeatureOptionLock : IFeatureOptionRepository
{
    private readonly IFeatureOptionRepository _options;
    private readonly IImmutableSet<string> _lockedFeatures;

    public FeatureOptionLock(IFeatureOptionRepository options, IEnumerable<string> lockedFeatures)
    {
        _options = options;
        _lockedFeatures = lockedFeatures.ToImmutableHashSet(SoftString.Comparer);
    }

    public bool IsDirty(string name) => _options.IsDirty(name);

    public FeatureOption GetOption(string name)
    {
        return _options.GetOption(name);
    }

    public FeatureOption UpdateOption(string name, Func<FeatureOption, FeatureOption> configure)
    {
        if (_lockedFeatures.Contains(name))
        {
            throw new InvalidOperationException($"Cannot configure feature '{name}' because it's locked.");
        }

        return _options.UpdateOption(name, configure);
    }
}

The default implementation of FeatureToggle is now trivial:

public class FeatureToggle : IFeatureToggle
{
    private readonly ILogger _logger;

    public FeatureToggle(ILogger<FeatureToggle> logger, IFeatureOptionRepository options)
    {
        Options = options;
        _logger = logger;
    }

    public IFeatureOptionRepository Options { get; }

    public async Task<T> ExecuteAsync<T>(string name, Func<Task<T>> body, Func<Task<T>> fallback)
    {
        // Not catching exceptions because the caller should handle them.
        return
            this.IsEnabled(name)
                ? await body().ConfigureAwait(false)
                : await fallback().ConfigureAwait(false);
    }
}

But in case I want to log some statistics about features, I'll just decorate it with the new FeatueTelemetry:

public class FeatureTelemetry : IFeatureToggle
{
    private readonly ILogger _logger;
    private readonly IFeatureToggle _featureToggle;

    public FeatureTelemetry(ILogger<FeatureTelemetry> logger, IFeatureToggle featureToggle)
    {
        _logger = logger;
        _featureToggle = featureToggle;
    }

    public IFeatureOptionRepository Options => _featureToggle.Options;

    public async Task<T> ExecuteAsync<T>(string name, Func<Task<T>> body, Func<Task<T>> fallback)
    {
        using (_logger.BeginScope().CorrelationHandle("Feature").AttachElapsed())
        {
            _logger.Log(Abstraction.Layer.Service().Meta(new { FeatureName = name }).Trace());

            if (_featureToggle.Options.IsDirty(name))
            {
                _logger.Log(Abstraction.Layer.Service().Decision("Using custom feature options.").Because("Customized by user."));
            }

            return await _featureToggle.ExecuteAsync(name, body, fallback);
        }
    }
}

Bonus - feature keys

Here's an example of how I create featue keys with my key-generator:

namespace Features
{
    [UseType, UseMember]
    [PlainSelectorFormatter]
    public class DemoFeatures : SelectorBuilder<DemoFeatures>
    {
        public static StringSelector<object> Greeting { get; } = Select(() => Greeting).AsString();

        [Tags("io")]
        public static StringSelector<object> ReadFile { get; } = Select(() => ReadFile).AsString();
    }

    [UseType, UseMember]
    [PlainSelectorFormatter] // todo - comment out to trigger selector-formatter-not-found-exception
    public class DatabaseFeatures : SelectorBuilder<DatabaseFeatures>
    {
        [Tags("io")]
        public static StringSelector<object> Commit { get; } = Select(() => Commit).AsString();
    }
}

I use them the same way any any other readonly static members but they give me a lot more flexibility in ensuring consistent naming, providing T in other use-cases, tagging and reflection.

    [Fact]
    public void Can_configure_features_by_tags()
    {
        var options =
            new FeatureOptionRepository()
                .DecorateWith(o => new FeatureOptionFallback(o, FeatureOption.Enable | FeatureOption.Warn | FeatureOption.Telemetry));

        var features = new FeatureToggle(Logger<FeatureToggle>.Empty, options);

        var names =
            ImmutableList<Selector>
                .Empty
                .AddFrom<DemoFeatures>()
                .AddFrom<DatabaseFeatures>()
                .Where<TagsAttribute>("io")
                .Format();

        features.Configure(names, o => o.Reset(FeatureOption.Enable));

        var bodyCounter = 0;
        var otherCounter = 0;
        features.Execute(DemoFeatures.Greeting, () => bodyCounter++, () => otherCounter++);
        features.Execute(DemoFeatures.ReadFile, () => bodyCounter++, () => otherCounter++);
        features.Execute(DatabaseFeatures.Commit, () => bodyCounter++, () => otherCounter++);

        Assert.Equal(1, bodyCounter);
        Assert.Equal(2, otherCounter);
    }
\$\endgroup\$
  • \$\begingroup\$ This might even be a follow-up... \$\endgroup\$ – t3chb0t Jul 25 at 8:05
  • 1
    \$\begingroup\$ @dfhwze sure, we should definitely wait a couple more days. Thanks for attracting attention to this question. I was too greedy ;-) \$\endgroup\$ – t3chb0t Jul 25 at 8:32
  • 2
    \$\begingroup\$ It's ok, we know you breathe C# code like we do air ;-) \$\endgroup\$ – dfhwze Jul 25 at 8:35
  • 1
    \$\begingroup\$ I mean could you combine the task functions to only call ConfigureAwait once? :p and it's missing in FeatureTelemetry I think \$\endgroup\$ – dfhwze Jul 27 at 13:32
  • 1
    \$\begingroup\$ oops, I'll try again: I use ConfigureAwait here twice because I have no idea why I didn't see this one :-] \$\endgroup\$ – t3chb0t Jul 27 at 13:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.