New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make IoC/DI friendlier #108

Merged
merged 27 commits into from May 29, 2018

Conversation

Projects
None yet
6 participants
@martincostello
Contributor

martincostello commented May 19, 2018

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.

martincostello added some commits May 19, 2018

Pass publisher through to experiments
Push the global IResultPublisher instance from Scientist through to ExperimentInstance via ExperimentSettings.
Remove shared state
Make each InMemoryResultPublisher instance have its own state, rather than each instance just wrap shared static state.
Add "Professor" class
Add a "Professor" class, which provides the same functionality as Scientist, but as an instance-based class, rather than a static class.
This is to make activation via dependency injection easier by removing the need to set a shared static property with the desired IResultPublisher instance that might require activation via an IoC container.
Add ItemGroup to test project
Add the ItemGroup that Visual Studio 2015 and later insist on adding to test projects.
@davezych

This comment has been minimized.

Contributor

davezych commented May 21, 2018

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

This comment has been minimized.

Contributor

martincostello commented May 21, 2018

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

This comment has been minimized.

Contributor

Haacked commented May 21, 2018

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

This comment has been minimized.

Contributor

martincostello commented May 21, 2018

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)?

Bump AppVeyor OS and .NET Core SDK
Bump AppVeyor CI OS to VS 2017 and the .NET Core SDK to 2.1.200 to see if that fixes the build not running propertly anymore.
@Haacked

This comment has been minimized.

Contributor

Haacked commented May 21, 2018

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

This comment has been minimized.

Contributor

martincostello commented May 21, 2018

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

martincostello added some commits May 22, 2018

Update to latest .NET Core test dependencies
Update .NET Core test SDK and xunit to the latest stable versions.
Bump tests to target netcoreapp2.0.
Try and fix AppVeyor build
Try and fix the AppVeyor build by adjusting the indenting for the build script.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

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

This comment has been minimized.

Contributor

ryangribble commented May 22, 2018

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 added some commits May 22, 2018

Refactor Scientist into a non-static class
Refactor Scientist to support creating instances to support use with IoC containers.
Existing static methods defer to a private singleton instance of Scientist.
Add IScientist which contains the two most configurable methods from Scientist, and add extension methods to add the convenience overloads.
Reset AppVeyor YAML
Reset the AppVeyor YAML file back to master.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

@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.

Use Visual Studio 2017 to build in AppVeyor
Explicitly use the Visual Studio 2017 AppVeyor image.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

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

This comment has been minimized.

Contributor

ryangribble commented May 22, 2018

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

image

martincostello added some commits May 22, 2018

Test a theory
Test a theory about the AppVeyor YAML by adjusting the formatting and only running one build command for now.
Restore test and package commands
Restore the commands to run the tests and create the package to AppVeyor YAML.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

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.

Fix incorrect exception argument order
Fix incorrect order of parameters to exception in Scientist constructor.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

@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 added some commits May 22, 2018

Fix instance state not being used
Fix the state of Scientist instances not being used to build experiments.
Add unit tests for instance behaviour
Add unit tests for using instances of Scientist to run experiments.
Remove Swap.Publisher()
Remove Swap.Publisher() as it is no longer used.
Reduce use of Scientist.ResultPublisher
Refactor an instance unit test to not depend on the shared results publisher.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 22, 2018

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
Fix accidentally commented-out code
Restore a line that was commented-out accidentally from 06935a6.
@tnederveld

This comment has been minimized.

tnederveld commented May 25, 2018

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

Thanks for taking this on.

@martincostello

This comment has been minimized.

Contributor

martincostello commented May 25, 2018

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

@Haacked

This comment has been minimized.

Contributor

Haacked commented May 25, 2018

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

@Haacked

Just a couple nitpicky things. Overall looks good.

public Scientist(IResultPublisher resultPublisher)
{
if (resultPublisher == null)
throw new ArgumentNullException(nameof(resultPublisher), "A result publisher must be specified");

This comment has been minimized.

@Haacked

Haacked May 25, 2018

Contributor

Here's an opportunity to use the new C# throw expressions to make the code even simpler.

_resultPublisher = resultPublisher
  ?? throw new ArgumentNullException(nameof(resultPublisher), "A result publisher must be specified");

This comment has been minimized.

@martincostello

martincostello May 26, 2018

Contributor

Changed.

@@ -84,7 +117,7 @@ public static T Science<T>(string name, Action<IExperiment<T>> experiment)
/// <param name="experiment">Experiment callback used to configure the experiment</param>
/// <returns>The value of the experiment's control function.</returns>
public static Task<T> ScienceAsync<T>(string name, Action<IExperimentAsync<T>> experiment) =>
ScienceAsync(name, 1, experiment);
_sharedScientist.Value.ExperimentAsync(name, experiment);

This comment has been minimized.

@Haacked

Haacked May 25, 2018

Contributor

Looks like a stray character of whitespace was added here.

This comment has been minimized.

@martincostello

martincostello May 26, 2018

Contributor

There were a couple of these in this class, all adjusted now.

Address review feedback
Make parameter validation more terse.
Remove rogue whitespace.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 26, 2018

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

This comment has been minimized.

Contributor

joncloud commented May 26, 2018

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

martincostello added some commits May 27, 2018

Use .NET Core 2.1.300 SDK
Update to the .NET Core 2.1 SDK.
Add SourceLink support
Add support for SourceLink so users can step-into Scientist itself in the applications they integrate it into.
Remove redundant build arguments
Remove redundant arguments from the build scripts.
Update version to 2.0.0.0
Bump version to 2.0.0.0 (alpha1).
Update release notes
Add bare-bones entry for 2.0.0 to ReleaseNotes.md.
@martincostello

This comment has been minimized.

Contributor

martincostello commented May 27, 2018

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 e15e847 won't light up until the newer SDK is used.

@Haacked Haacked merged commit 8e4801f into github:master May 29, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Haacked

This comment has been minimized.

Contributor

Haacked commented May 29, 2018

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. 😄

Thanks for the contribution @martincostello!

@martincostello martincostello deleted the martincostello:Static-To-Not branch May 29, 2018

@martincostello

This comment has been minimized.

Contributor

martincostello commented May 29, 2018

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

This comment has been minimized.

Contributor

Haacked commented May 29, 2018

sigh I need to spend some time looking into this.

@martincostello

This comment has been minimized.

Contributor

martincostello commented May 31, 2018

@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

This comment has been minimized.

Contributor

Haacked commented Jun 1, 2018

@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

This comment has been minimized.

Contributor

martincostello commented Jun 1, 2018

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

@Haacked

This comment has been minimized.

Contributor

Haacked commented Jun 1, 2018

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

@tnederveld

This comment has been minimized.

tnederveld commented Jun 1, 2018

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

@martincostello

This comment has been minimized.

Contributor

martincostello commented Jun 5, 2018

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

@Haacked

This comment has been minimized.

Contributor

Haacked commented Jun 5, 2018

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

This comment has been minimized.

Contributor

martincostello commented Jun 5, 2018

Awesome - thanks for your help @Haacked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment