Require script_id for SingleSectionExperiment and TeacherBasedExperiment

[davidsbailey]

When we added sections 20-40 to the ai-rubrics experiment, we discovered that the presence of so many SingleSectionExperiment objects in production was causing high DB usage, because we would look up the sections of the current user on every page load. Details here: https://codedotorg.slack.com/archives/C0T0PNTM3/p1698947153623759?thread_ts=1698936572.562019&cid=C0T0PNTM3

We mitigated the problem by adding script_id to every SingleSectionExperiment in the DB. this works because the main hot codepath for looking up user experiments does not specify a script_id:

• code-dot-org/dashboard/config/initializers/devise.rb

  Line 361 in a1a2775

  auth.cookies[environment_specific_cookie_name("_experiments")] = {value: user.get_active_experiment_names.to_json, domain: :all}

• code-dot-org/dashboard/app/models/user.rb

  Lines 1691 to 1692 in 97abdf8

  	def get_active_experiment_names
  	Experiment.get_all_enabled(user: self).pluck(:name)

Which causes the experiment lookup to short circuit without querying the database, here:

code-dot-org/dashboard/app/models/experiments/experiment.rb

Line 62 in 51d2f42

(experiment.script_id.nil? || experiment.script_id == script.try(:id)) &&

This PR generalizes the solution by requiring that SingleSectionExperiment and TeacherBasedExperiment have a script_id, causing DB lookups to be skipped in most cases.

It's worth noting that there will still be some DB activity for each SingleSectionExperiment, because users who are viewing levels in the unit matching the script_id of any SingleSectionExperiment will trigger DB access, here:

• code-dot-org/dashboard/app/helpers/levels_helper.rb

  Line 398 in bca7559

  Experiment.get_all_enabled(user: current_user, script: @script).pluck(:name)

• code-dot-org/dashboard/app/models/experiments/experiment.rb

  Line 63 in 51d2f42

  experiment.enabled?(user: user)

• code-dot-org/dashboard/app/models/experiments/single_section_experiment.rb

  Line 39 in d96ebb3

  user.sections.include?(section) || user.sections_as_student.include?(section)

However, empirical evidence suggests that we have sufficient headroom in database CPU that this won't become a problem with moderate numbers of SingleSectionExperiment objects, especially if they are assigned to units which are not frequently visited (i.e. not HOC units).

Testing story

added / updated unit tests

Follow-up work

open to ideas here

[bethanyaconnor]

LGTM!

As a future future improvement, I wonder if we should clean up these rows when the pilot is over. The problem isn't mitigates if we have many experiments across multiple scripts -- that doesn't feel likely to happen all at once, but could be a long-term issue.

[davidsbailey]

I agree, we can't be sure this issue is totally behind us. I can set a reminder to remove these, but I think a greater risk than forgetting about these records is that we create too many of them in the first place. For example, the open pilot is currently estimated to have 11,000 sections in it, which seems like too many for this implementation. Would it help to add a validation that there can't be more than, say, 1000 instances of SingleSectionExperiment for a given experiment name? https://stackoverflow.com/questions/1270663/validate-max-amount-af-associated-objects

[bethanyaconnor]

That could be helpful, but I think it's worse if we have a lot of different experiments across different scripts, right? That would result in a larger percentage of requests checking for inclusion in these experiments, possibly leading to the congestion we saw on the database. So I wonder if it makes more sense to have a strict limit on the number of rows across all experiments?

Either way, this change is definitely necessary, based on the issues we saw.

[davidsbailey]

I would argue the negative impact is proportional to the number of SingleSectionExperiments with a given script id times the frequency with which users load that script . The experiment name isn't particularly relevant from a technical standpoint, rather it seemed like a way to add a limit while avoiding the situation where one team's experiment could interfere with another's.

I think you're right that a total limit on the number of SingleSectionExperiment rows is probably better though. If a team is in a pinch, they can always bypass the validation to create row 1001 while they regroup and decide what to do next.