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

Preloading features #198

Merged
merged 8 commits into from Nov 21, 2016

Conversation

Projects
None yet
3 participants
@gshutler
Contributor

gshutler commented Nov 17, 2016

Some code to help the discussion in #190


When N features are being checked in a single request this would result in N requests to the underlying store at best with memoization enabled.

This PR adds get_multi to the adapter protocol, backfilled to all adapters through the Flipper::Adapter module implementing it as multiple get calls.

This allows adapters to optimize get_multi when possible to improve the performance of retrieving multiple flags.

To take advantage of this preload was added to the DSL, which allows the specified features to be eagerly memoized. New configuration options were added to the memoizing middleware to take advantage of this.

get_multi is currently only implement for the Memoizable and Redis adapters.

gshutler added some commits Nov 17, 2016

Allows features to be retrieve in bulk
When N features are being checked in a single request this would result
in N requests to the underlying store at best with memoization enabled.

This commit adds `get_multi` to the adapter protocol, backfilled to all
adapters through the Flipper::Adapter module implementing it as multiple
`get` calls.

This allows adapters to optimize `get_multi` when possible to improve
the performance of retrieving multiple flags.

To take advantage of this `preload` and `preload_all` were added to the
DSL, which allow the specified features to eagerly memoized. A new eager
version of the memoizing middleware was added to demonstrate this.

`get_multi` is currently only implement for the Memoizable and Redis
adapters.
@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 18, 2016

We've starting running this in production and it seems to be having the desired effect.

@jnunemaker

Super helpful to see this. I definitely got a few ideas by having some code in front of me. I like the overall approach. I am keen to hear your thoughts on the middleware specifically.

Also, I would love to see tests for preload, preload_all, get_multi and the new middleware (or whatever we go with there). Do you have time to add those? If not, I can work on seeing this through, but it might be a bit before I have time.

I am definitely down to merge this with tests and a few minor tweaks and I really appreciate the thoughtful PR.

@@ -0,0 +1 @@
2.3.0-dev

This comment has been minimized.

@jnunemaker

jnunemaker Nov 19, 2016

Owner

What are your thoughts on removing this and dropping .ruby-version in .gitignore?

This comment has been minimized.

@gshutler

gshutler Nov 19, 2016

Contributor

Sure, I didn't mean to commit it in the first place!

features
end
def preload_all

This comment has been minimized.

@jnunemaker

jnunemaker Nov 19, 2016

Owner

I'm on the fence about encouraging this as it could become equally as damaging as getting things one by one, but I can always add a caveat to the caveats.md file about being careful to not have too many features while using preload_all.

This comment has been minimized.

@gshutler

gshutler Nov 19, 2016

Contributor

I understand, I think we'll be able to get rid of it.

module Flipper
module Middleware
class EagerMemoizer

This comment has been minimized.

@jnunemaker

jnunemaker Nov 19, 2016

Owner

What do you think about calling this Flipper::Middleware::PreloadAll?

Also, what do you think about having the preload all memoizer only call preload_all. We could then recommend including the memoizer first in the stack and PreloadAll second. It would keep logic in the same place.

In fact, I think we could even set an env var like flipper.memoizing in the memoizing middleware and then log a warning to stderr or something here if it is missing, as a way of letting people know they are preloading without memoization.

Another option, we could just add a preload_all option to the memoizing middleware so people could just do:

use Flipper::Middleware::Memoizer, preload_all => true

# or 
use Flipper::Middleware::Memoizer, preload: [:my, :collection, :of, :features, :to, :preload]

This comment has been minimized.

@gshutler

gshutler Nov 19, 2016

Contributor

I like the idea of making it into an option of Memoizer as it would avoid them being able to be registered in the wrong order and so on.

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Nov 19, 2016

Also, if you don't mind, I would love to know what you are using flipper for as well and how it is working out for you (what went well, what was hard). If you aren't comfortable leaving that information here in a public comment, feel free to email me directly (it shows up on my profile page). I'm just always curious. :)

@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 19, 2016

Happy to write some tests, I also plan to have a look at how to implement get_multi for the other bundled adapters.

We use Flipper at Cronofy for all sorts. Hiding UI features, easing in new features to our synchronisation layer, making new endpoints/data available to specific clients during development, experimenting with new implementations of things that should be transparent.

The big thing that triggered this work for us is we're changing our data storage and so rather than generally 4-5 flags in play at any time we've got 20+. We were seeing roughly 1ms per feature flag fetch and with API endpoints that respond in around 90ms adding 20ms really shifted the needle. Using this approach has led to ~4ms on every request.

Flipper was easy to get going with, probably the fiddliest thing was wrapping things to expose a flipper_id when wanting to work on an actor basis. Might be nice to be able to pass a String rather than have to wrap an expose flipper_id? Occasionally it would have been nice to be able to see a history of the the feature flag's setting changes.

Any other feedback from our use @stephenbinns?

@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 19, 2016

@jnunemaker I think this addresses your key points. I'm not sure if I've added tests in all the places you would want them or not so any pointers you can give would be good.

I also plan to look into implementing get_multi for the other adapters at some point but it's emulated by multiple get calls within Flipper::Adapter at the moment so I don't think any of them should be broken.

@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 19, 2016

Worked out where to add the spec that confirms get_multi works for all adapters.

@jnunemaker jnunemaker merged commit 037fc8f into jnunemaker:master Nov 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
response = @app.call(env)
response[2] = Rack::BodyProxy.new(response[2]) {
flipper.adapter.memoize = original
}
response
rescue

This comment has been minimized.

@jnunemaker

jnunemaker Nov 21, 2016

Owner

One question: what was this added for?

This comment has been minimized.

@gshutler

gshutler Nov 21, 2016

Contributor

One of the spec groups I added failed one of the shared examples. This resolved that failure but I couldn't really see why it was needed if I'm honest. If you take it out you'll see the failure and perhaps be able to solve it some other way.

This comment has been minimized.

@gshutler

gshutler Nov 22, 2016

Contributor

Ah, I see from bdbee31 I was misreading the BodyProxy, thinking it was wrapping the whole @app.call not just the body. Makes sense now why it was needed.

@stephenbinns

This comment has been minimized.

stephenbinns commented Nov 22, 2016

Sorry just got round to replying to this I don't really have much to add to what @gshutler's already said about our usage. The defaults are sensible and we've had no real issues other than when we've introduced a large number of flags.

One thing we've noticed from our usage is we rarely use the 75% enabled on the UI as we generally would tend towards going from 50% -> 100% (as usually by 50% you're pretty committed).

For us the biggest feature gap is the audit trail (I see its already on the issues list) we use Slack for most of our integrations, so we might have a look into hooking into the UI to send notifications on flag changes.

@jnunemaker jnunemaker referenced this pull request Nov 25, 2016

Closed

Cleanup work for Adapter#get_multi #206

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