Skip to content

Check environment for experiment first

Doug Stull requested to merge move-experiment-env-check-first into master

What does this MR do?

  • Switches the order of operations for checking if the Experiment is enabled so that the typically less expensive(no db query) operation is checked first instead of reaching out to Flipper.get first.

When would this help performance - even if just slightly?

  • The default environment check is used Gitlab.dev_env_or_com? and GitLab instance is self-managed as it would not perform the Feature.get check.
  • Used on controllers where the branching logic is in the view for determining if the experiment should be run, so we have to perform record at the lower controller level on every request !45689 (diffs)
    • Implementing it like this will alleviate the need to gate as above to reduce DB queries.

Here are some numbers - note that typically this is a one time run, so typical perf analysis using Benchmark ips seemed off due to rails db layer caching after the first run.

  • gdk restart to clear rails db cache
  • Running old way, then new way as introduced in this MR shows new way faster
[1] pry(main)> experiment = Gitlab::Experimentation.experiment(:suggest_pipeline)
=> #<struct Gitlab::Experimentation::Experiment key=:suggest_pipeline, environment=nil, tracking_category="Growth::Expansion::Experiment::SuggestPipeline">
[2] pry(main)> Benchmark.measure { experiment.enabled?  && experiment.enabled_for_environment? }
  Feature::FlipperGate Load (0.8ms)  SELECT "feature_gates".* FROM "feature_gates" WHERE "feature_gates"."feature_key" = $1  [["feature_key", "suggest_pipeline_experiment_percentage"]]
=> #<Benchmark::Tms:0x00007fbee08f9b10 @cstime=0.0, @cutime=0.0, @label="", @real=0.07522700005210936, @stime=0.017879000000000644, @total=0.06068400000000196, @utime=0.042805000000001314>
[3] pry(main)> Benchmark.measure { experiment.enabled_for_environment? && experiment.enabled? }
=> #<Benchmark::Tms:0x00007fbee09191e0 @cstime=0.0, @cutime=0.0, @label="", @real=3.400002606213093e-05, @stime=1.4000000000180535e-05, @total=3.8000000003535206e-05, @utime=2.400000000335467e-05>
  • gdk restart to clear rails db cache
  • Running new way, then old way as introduced in this MR shows new way faster
[1] pry(main)> experiment = Gitlab::Experimentation.experiment(:suggest_pipeline)
=> #<struct Gitlab::Experimentation::Experiment key=:suggest_pipeline, environment=nil, tracking_category="Growth::Expansion::Experiment::SuggestPipeline">
[2] pry(main)> Benchmark.measure { experiment.enabled_for_environment? && experiment.enabled? }
=> #<Benchmark::Tms:0x00007f80d2944c10 @cstime=0.0, @cutime=0.0, @label="", @real=3.700004890561104e-05, @stime=1.4000000000180535e-05, @total=4.099999999951365e-05, @utime=2.6999999999333113e-05>
[3] pry(main)> Benchmark.measure { experiment.enabled?  && experiment.enabled_for_environment? }
  Feature::FlipperGate Load (0.9ms)  SELECT "feature_gates".* FROM "feature_gates" WHERE "feature_gates"."feature_key" = $1  [["feature_key", "suggest_pipeline_experiment_percentage"]]
=> #<Benchmark::Tms:0x00007f80d3bf9978 @cstime=0.0, @cutime=0.0, @label="", @real=0.03276399988681078, @stime=0.002685999999999744, @total=0.018488000000000504, @utime=0.01580200000000076>
  • Running old way one more time w/o clearing db cache - it is faster than new way...but is rails db layer caching reliable? 🤷 
[4] pry(main)> Benchmark.measure { experiment.enabled?  && experiment.enabled_for_environment? }
=> #<Benchmark::Tms:0x00007f80c45590c8 @cstime=0.0, @cutime=0.0, @label="", @real=7.00005330145359e-06, @stime=2.9999999995311555e-06, @total=1.0999999997096666e-05, @utime=7.99999999756551e-06>

In the case of

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Merge request reports