Make IoC/DI friendlier

[martincostello]

This PR is very much a proposal for discussion rather than a fait accompli, so is presented more in a "does the job" fashion.

This week I plugged Scientist into an ASP.NET Core application for use with running an experiment on part of the code, and as such would have liked to register my IResultPublisher for use via dependency injection so it is only activated when needed and my implementation can be activated the same way for its dependencies. However, due to the shared static Scientist.ResultPublisher property, this isn't really possible to do in a nice way.

In the end I settled on setting up IResultPublisher in IoC as a singleton where the factory method to create the service set the resolved implementation as the value of Scientist.ResultPublisher before it returned, and then made the code where I wanted to run the experiment have an unused IResultPublisher constructor parameter to force the IoC container to set it up so it was created on-demand.

This PR introduces a new Professor class which is instance-based, rather than static, and provides the same functionality as the Scientist class. The name of the class is arbitrary and I just picked something "sciency" that didn't need the existing Scientist class to be changed and be a breaking change.

Professor accepts an IResultPublisher as a constructor parameter, allowing the running of the experiment to be decoupled from ownership and activation of the publisher itself, so it can be managed by an IoC container, for example. It also moves the concurrent tasks to a property to reduce the number of methods present on the class, but the overloads could always be restored for parity.

Below are some example snippets of code from an ASP.NET Core 2.1 application leveraging Professor to run experiments using DI for requests to a controller that are published to another HTTP service.

// Startup.cs
public void ConfigureServices(IServiceCollection services)
{
    // Other IoC registration...
    // Register HttpClient and configure for use with our results publisher
    services.AddHttpClient<IResultPublisher, HttpResultsPublisher>((p) => p.BaseAddress = new Uri("https://collector.local/"));
    // Create a new Scientist for each need of it (could also be a Singleton or Scoped)
    services.AddTransient<IScientist, Scientist>();
}

// HttpResultsPublisher.cs
public class HttpResultsPublisher : IResultPublisher
{
    private readonly HttpClient _httpClient;

    public HttpResultsPublisher(HttpClient httpClient)
    {
        _httpClient = httpClient;
    }

    public async Task Publish<T, TClean>(Result<T, TClean> result)
    {
        // POST JSON-serialized result to an external HTTP data-collection service
        using (var response = await _httpClient.PostAsJsonAsync("experiment", result))
        {
            response.EnsureSuccessStatusCode();
        }
    }
}

// MeaningOfLifeController.cs
[Route("meaning-of-life")]
public class MeaningOfLifeController : Controller
{
    private readonly IScientist _scientist;

    public MeaningOfLifeController(IScientist scientist)
    {
        _scientist = scientist;
    }

    [HttpGet]
    public IActionResult Get()
    {
        int result = _scientist.Experiment<int>("Meaning of Life", (experiment) =>
        {
            experiment.Use(() => 42);
            experiment.Try(() => 37);
        });

        return Json(new { value = result });
    }
}

I've also changed InMemoryResultPublisher to be backed by instance fields so that each instance has its own results. This was mainly to make it easier to duplicate the tests for Scientist for Professor without the state clashing. This change could be reverted, but would require some rework in the tests for Professor to use a different default results implementation so it doesn't affect the Scientist tests.

[davezych]

I took a very brief look at this and overall I think it's a good change and I can see it being useful. (I'm hoping to circle back in a few hours and take a deeper look but we'll see how my day goes.)

My only comment right now is about Professor - it looks like a complete, but non static, copy of Scientist. I hate having that duplication because it will make maintenance a pain, and potentially adds confusion for any devs trying to use it. Can we somehow get away with a single implementation of Scientist to achieve both functionalities? Perhaps a non static class with a static Instance property?

[martincostello]

Yeah that's definitely an approach we could take, and yeah it pretty much is a copy. For now, I did that as a way of not making a breaking change to the interface and make the diff a bit cleaner for conceptual review.

Bumping the version to 2.0.0 for SemVer would allow the changes to be an effective refactor of Scientist instead and remove the duplication if that's a workable option.

[haacked]

I like where this is heading. Here's a thought that I don't think would require breaking changes. The way the Scientist class is written today is to allow developers to use it quickly with almost zero configuration and remove it easily when they're done with configuration. Hence the static implementation.

However, for developers who plan to always have experiments running and have invested in existing DI and IoC, I definitely see the benefit of making the library more friendly to those approaches.

So here's a proposal:

1. Make the Scientist class non-static. But keep the existing static methods.
2. Add a new IScientist interface and make the Scientist class implement it. (We'll have to tweak the names on the interface to not conflict with the existing static methods)
3. The static methods should all delegate to a private lazily instantiated static instance of Scientist.

This way, existing users code will continue to just work. But users who want to use DI/IoC will register and import an IScientist instance and completely ignore the whole static side of things.

We could consider deprecating all the static methods later if we wanted to.

[martincostello]

I did consider doing the "make the static defer to a singleton instance" approach, but I thought I'd get the conversation started first as that was more work than the initial "Professor" commit.

I like the proposal above @Haacked, I guess we'd just have to figure exactly what we want to make as the interface members, and what would be convenience extras. For example, we could make the methods that take the concurrentTasks parameter as the interface members and put the convenience 1 overloads as extension method(s)?

I'm not sure whether or not making a static class not is a breaking change from a binary drop-in point-of-view for the IL of existing calls, but yeah it certainly could be a non-breaking code change for upgrade and recompile.

I think the only thing that would need a bit more thought is how to support the changing the IResultsPublisher in the static property into the private singleton? While the documentation states it should be set once before use, it technically does allow it to be changed at runtime. Would this be just a "hacky" internal method that allows the static to change the implementation at runtime and nothing else, or would we change it to only support setting before first use and change to throw an exception after that (which would be a breaking behavioural change)?

[haacked]

Would this be just a "hacky" internal method that allows the static to change the implementation at runtime and nothing else, or would we change it to only support setting before first use and change to throw an exception after that (which would be a breaking behavioural change)?

Yeah, I think this is right.

Note that under my proposal, if you set up IoC in a project as your means to using Scientist, but you accidentally call a static method, you might be operating on two different instances of Scientist. So it's important that you stick with one approach or another.

That's probably fine for now.

[martincostello]

I’ll update this PR with a first cut of the proposed updates tomorrow.

[martincostello]

The AppVeyor CI appears to be ignoring the custom build script and is just trying to run vanilla MSBuild on the solution file instead. I've tried to fix it, but I'm a bit baffled as to what's going on...

[ryangribble]

I'm not sure how the diff on this pull request is showing the previous version of the appveyor file to have weirs things like double hyphens but if you look at the original file compared to what you've got, you should be able to restore the yml formatting

https://github.com/github/Scientist.net/blob/master/appveyor.yml

[martincostello]

@ryangribble I've reset the YAML file, but while the build appears to be green, if you look at it it doesn't actually build anything. It's as if AppVeyor is completely ignoring the existence of the file.

[martincostello]

It looks like it's ignoring the build_scripts commands, as changing to the VS 2017 image has taken an effect, and now it fails because dotnet restore hasn't been run when it tries to do the default build.

[ryangribble]

hmm ok, i was going to say perhaps in the appveyor settings the "ignore appveyor.yml" option is enabled (only @Haacked would be able to confirm that) however if that change is having an effect I guess that wouldn't be it

[martincostello]

I think maybe that the build setting was somehow making build_script be ignored. Seems to be fixed now. Looks like the packages are just going into the wrong folder so not being published now.

[martincostello]

@Haacked Have done an initial pass at changing Scientist as per your suggestion. Just adding some specific unit tests for the instance-based implementation now.

[martincostello]

Summary of latest changes:

• Added IScientist interface with methods:
  1. T Experiment<T, TClean>(string name, Action<IExperiment<T, TClean>> experiment)
  2. Task<T> ExperimentAsync<T, TClean>(string name, int concurrentTasks, Action<IExperimentAsync<T, TClean>> experiment)
• Added extension methods for IScientist which add convenience methods for the equivalent overloads on the static methods.
• Scientist now implements IScientist.
• Static methods on Scientist defer to a private singleton instance of Scientist for their implementation which proxies the state of the static Enabled() methods.
• Scientist.ResultPublisher now throws if an attempt is made to set it after the shared singleton has been created.

[martincostello]

Must remember to update README once the final changes are ironed-out...

[martincostello]

Results from benchmarking a static method against using the equivalent instance method:

BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=2.1.300-rc1-008673
  [Host]     : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.7 (CoreCLR 4.6.26328.01, CoreFX 4.6.26403.03), 64bit RyuJIT

Method	Mean	Error	StdDev	Scaled	ScaledSD	Gen 0	Gen 1	Allocated
Static	8.196 us	0.1626 us	0.2483 us	1.00	0.00	0.7782	0.1984	4.74 KB
Instance	8.213 us	0.1560 us	0.1916 us	1.00	0.04	0.7782	0.2747	4.74 KB

[tnederveld]

When can we expect to see this PR merged? Been toying with it locally, and works well.

Thanks for taking this on.

[martincostello]

I'm just waiting on further review since I refactored after the initial comments.

[haacked]

Excellent! I need some time to review it. I’ll try and get to it soon.

[martincostello]

Assuming no further code feedback, I think the last things to do are:

1. Update README.
2. Bump the version to 2.0.0.
3. Fix whatever has changed that means AppVeyor isn't publishing the nupkg.

[joncloud]

Awesome changes - I'm excited to see improved integration with DI!

[martincostello]

I tried to update the SDK to 2.1.300, but it turned out the Fake build didn't work the way I expected and those parameters were redundant, so I've removed those. However, that means the SourceLink support I added in e15e847028882fd95529f80ecffc749e34a07a2a won't light up until the newer SDK is used.

[haacked]

I can't remember if I have this set up to automatically publish to NuGet on merge to master. I really hope I do because I'm lazy. :smile:

Thanks for the contribution @martincostello!

[martincostello]

Looks like AppVeyor moved the 🧀 since the last changes to Scientist @Haacked, so the NuGet packages aren't being hoovered up in the build for publishing.

[haacked]

sigh I need to spend some time looking into this.

[martincostello]

@Haacked I appreciate you've got plenty on, just curious on some rough timelines for 2.0.0 being released to NuGet with the deferred publisher and these changes?

[haacked]

@martincostello I just published a 2.0.0-beta to NuGet (it's still validating right now). Please try it out and if it works for you, I'll publish a stable version of the package.

Thanks!

[martincostello]

@Haacked Awesome! I’ll give it a spin on Monday.

[haacked]

Great! It's available now if anyone else wants to give it a spin.

[tnederveld]

I pull it later this weekend, and mess around with it.

[martincostello]

@Haacked I've pulled the 2.0.0-beta package into my app and it works nicely 😎

[haacked]

Thanks for the confirmation @martincostello! I published a stable version of 2.0.0 today. https://haacked.com/archive/2018/06/05/scientist-2-point-oh/

[martincostello]

Awesome - thanks for your help @Haacked!

[M-Zuber]

@martincostello do you happen to remember why you needed to subclass Scientist instead of just doing static Scientist CreateSharedInstance() => new Scientist(ResultPublisher);? (I think I can get a necromancer badge now 😄)

[martincostello]

I don't remember, but the comment on the class says "This class acts as a proxy to allow the static methods to set the state on an instance of Scientist."

[M-Zuber]

I do see that, but based on #129, it would seem that things work even without the sub class

[martincostello]

I didn't want changes to the global shared stuff to affect other instances as having global shared state (especially when you're trying to add in DI to make everything nice and isolated and testable), so that was mainly there to provide a measure of backwards compatibility to people using the old static way of doing things without affecting instances created the new way.

I think it came about from testing, because if tests ran in parallel they'd cause things to randomly fail when the shared state was being altered.