Skip to content
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

get_treatment and get_treatment_with_config interface for Split and Memory Adapters #83

Merged
merged 7 commits into from
Oct 12, 2023

Conversation

veeweeherman
Copy link
Contributor

@veeweeherman veeweeherman commented Oct 6, 2023

Context

INS-1225

As the Split Adapter is written currently, it's only capable of reading from Split flags the value of being on, off, or in the control group. But Split features can have many options for treatments. We need to update the Adapters to be able to just retrieve the treatment value, if we want a fuller use of SplitIO's capabilities.

Changes

  • expanding the ThisFeature::Adapters::SplitIo interface by adding #treatment_value and #treatment_config methods, which uses SplitIO's get_treatment_with_config method and returns its config or treatment name as value
  • ThisFeature::Flag#treatment_value method that wraps the adapter's calling of the same method
  • #treatment_value equivalent in ThisFeature::Adapters::Memory so this can be used in all environments
    • for treatment values to be configured, there is also a #enable_treatment!() method, which is similar to #on! which adds to the local storage of flag configurations and their required contexts (for which User id, etc)
    • I've purposely omitted ThisFeature::Adapters::Memory#treatment_config, because I wanted some feedback on the Memory#treatment_value implementation first
  • updates to README for Memory
  • if my understanding is correct, this should be a non-breaking change, because it only include additions to the gem
  • TODO: update version, when changes finalized

veeweeherman and others added 3 commits October 5, 2023 17:50
Co-authored-by: Heidi Tsang <tsa.heidi@gmail.com>
Co-authored-by: Heidi Tsang <tsa.heidi@gmail.com>
Co-authored-by: Vy Hoang Cu <vy.cu@hover.to>
@jhines-hover
Copy link

Depending on the reasoning for using get_treatment_with_config() we may need to eventually implement this method in this gem too.

Alternatively, we could have get_treatment (or whatever you rename it to) return an hash/object like:

{ 
  value: {{ treatment value }}, 
  config : { ... } 
}

So then it becomes (I didn't use symbols, but use whatever makes sense for Ruby):

ThisFeature.flag("gpa_cc_input_banner", current_user.id.to_s).get_treatment.value
ThisFeature.flag("HE_plan_for_dtc_org", @homeowner.id).get_treatment.config

- There are multiple use cases in Hyperion of SplitIO's method where it returns both the treatment and it's configuration. This commit adds methods to retrieve these values
- Not exactly related, but I see in SplitIO's docs there is a `get_treatments_with_config` (plural) method. We're not using this in Hyperion yet, that I can see, but I wonder if it would be useful down the line. I don't typically suggest to add functionality that won't be used yet, but it;s clear that for a year or so, the ThisFeature gem was not yet capable to handle our needs, which is why the antipattern use of ThisFeature (`ThisFeature.adapter_for(:split)&.send(:client)`) has been committed many times into Hyperion. I'm unsure of why a change to ThisFeature wasn't done first.
- adds `#enable_treatment!()` method to register a flag, to a specific context (such as a User or Org), with a treatment name
- adds `#treatment_value()` method to find the treatment name, for a flag and specific context
- specs
@veeweeherman veeweeherman changed the title draft WIP get_treatment interface for Split get_treatment and get_treatment_with_config interface for Split and Memory Adapters Oct 12, 2023
1. `flag_name`: String or Symbol (not a named argument)
2. `context`: User or Org object

When the Memory storage does not contain the given flag_name or if there is no provided `context`, `"control"` is returned. This is meant to mimic what SplitIO would return in the case of no configured treatment for the given `context`.
Copy link
Contributor Author

@veeweeherman veeweeherman Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See SplitIO's docs on their Control treatment

The Memory Adapter doesn't mimic SplitIO exactly. With SplitIO, depending on whether or not the flag has been configured or has been configured with treatments, you'll get either "control" or "control_treatment" respectively, as a response.

I didn't know if it was important to mirror this functionality exactly for our Memory Adapter. Because these methods, and the Memory Adapter itself, are meant for use in the test environment only. And of course, because SplitIO can change their output at any time.

Aside from that, there's other details I felt like a rabbit-hole discussion could've been started from. Such as returning false or nil, from a method that is meant to return 1 of potentially many values. Values that are not a simple on/off switch. Would false mean something different from nil, when there are options like v1, v2, v3, etc


```ruby
# If you have configured the in-memory adapter as the default
ThisFeature.test_adapter.enable_treatment!(:flag_name, treatment: 'treatment_name', context: user)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but I did notice it matter whether or not you pass the flag name as a symbol or string, in that it must be retrieved from the storage in the same way. May be a good for .with_indifferent_access on the storage hash

@veeweeherman veeweeherman merged commit 08a2c69 into main Oct 12, 2023
2 checks passed
@veeweeherman veeweeherman deleted the vy-heidi-get-treatment-interface branch October 12, 2023 23:04
veeweeherman added a commit that referenced this pull request Oct 13, 2023
[This ](#83 the only
change since the last version bump, so updating the Minor version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants