scientistproject / Scientist.net

A .NET library for carefully refactoring critical paths. It's a port of GitHub's Ruby Scientist library
MIT License
1.47k stars 95 forks source link

Implemented the ability to enable/disable experiments. #60

Closed joncloud closed 8 years ago

haacked commented 8 years ago

Remind me again what the difference between Enabled and RunIf is? They both seem to take a lambda that returns a boolean.

joncloud commented 8 years ago

Check out our discussion on #51.

haacked commented 8 years ago

Ah cool, I'll continue the discussion here with the PR.

Yea, Enabled is basically a specific case of RunIf.

In this code, it looks like it's exactly the same thing, not just a specific case. How is RunIf more generic than Enabled?

I haven't looked at the Ruby code. What's the semantic difference between RunIf and Enabled there? If the implementation is the same exact thing, I'd almost rather have Enabled and remove RunIf because I like the naming better.

joncloud commented 8 years ago

It looks like RunIf matches the functionality provided in ruby, but Enabled is driven through what seems like derivation of some sorts.

RunIf Example

class DashboardController
  include Scientist

  def dashboard_items
    science "dashboard-items" do |e|
      # only run this experiment for staff members
      e.run_if { current_user.staff? }
      # ...
  end
end

Enabled Example

class MyExperiment < ActiveRecord::Base
  include Scientist::Experiment
  def enabled?
    percent_enabled > 0 && rand(100) < percent_enabled
  end
end
haacked commented 8 years ago

Ah, I get it now. So yeah, it seems like this is one case where our implementation is different from the Ruby implementation because of differences in the languages.

Ruby allows you to override the new method so it's effectively a factory. That allows them to nicely replace any creation of Experiment with a custom MyExperiment class and then override the enabled method.

So the semantic difference here appears to be that Enabled is a global enabled, and RunIf is specific to a single experiment.

So with that in mind, I think there's two approaches we should consider.

  1. Don't implement Enabled. At least not in this way.
  2. Mimic the Ruby approach by allowing users to register their own Experiment type and create experiments via a factory method. Thus Enabled is a virtual method of the Experiment base class.

What do you think?

joncloud commented 8 years ago

Since Experiment builds an ExperimentInstance, how would it look to provide the call to Enabled? Would we still follow the pattern of providing a delegate to ExperimentInstance, but it would point to this.Enabled?

haacked commented 8 years ago

@joncloud sorry for the delayed response. It occurs to me that maybe the type that needs to be "registered" ala the Ruby approach is a custom implementation of ExperimentInstance, not Experiment given our current implementation.

I think the important point is that Enabled needs to be global. We don't absolutely have to follow the Ruby approach if we come up with a more idiosyncratic approach.

Unfortunately, I don't really have time to look at this deeply right now.

joncloud commented 8 years ago

Ah - That provides a bit more clarity to the requirement. Let me take a stab at it, and i'll post something up afterward.

joncloud commented 8 years ago

@davezych @Haacked let me know what you think.

joncloud commented 8 years ago

Whoops - closed this branch too early.

haacked commented 8 years ago

Thanks a lot! :sparkles: Nice work!