Avoid attaching to orders promotions that will never be eligible?

[elia]

What happened

A few months ago we faced an issue in production because the number of promotions in the system increased and we suddenly got a few dozens of them attached to every order. 😱

That with the one-page checkout we're currently employing that goes through all the steps all at once made the time taken to re-evaluate each of them exponentially higher with every additional promotion. 💥

you can se the latency go up, driven by calls to populate

How it was fixed

In the aftermath we realized the system is attaching to orders a number of promotions that are actually guaranteed to never be eligible, in our case any PromotionRule::User that is not connected with the user of the order and has match_policy: :all can be skipped, making the calls to recalculate much faster.

after drastically reducing the number of promotion-adjustments attached to orders we're back to normal

What about you?

You can check how many ineligible promotions you have attached to your orders with a query similar to this:

# not complete
Spree::Order.where.not(state: :complete).take(100).map{_1.adjustments.select(&:promotion?).reject(&:eligible?).count}.then{_1.sum/_1.size.to_f}
# => 3.14

# cart
Spree::Order.where(state: :cart).take(100).map{_1.adjustments.select(&:promotion?).reject(&:eligible?).count}.then{_1.sum/_1.size.to_f}
# => 6.49

# complete
Spree::Order.where(state: :complete).take(100).map{_1.adjustments.select(&:promotion?).reject(&:eligible?).count}.then{_1.sum/_1.size.to_f}
# => 0.33

What can we do?

We're now extending that optimization to most promotion rules to bring the number of ineligible promos from a constant number to nearly zero.

The technique is to filter the result of Spree::PromotionHandler::Cart#sale_promotions excluding those guaranteed to never be eligible, the code looks more or less like this:

      def sale_promotions
        Spree::Promotion
          .where(apply_automatically: true).active.includes(:promotion_rules).distinct
          .where.not(id: ::Spree::Promotion::Rules::User.excluded_promotions_for_order(order).select(:id))
      end

and can be generalized to:

      def sale_promotions
        scope = Spree::Promotion
          .where(apply_automatically: true).active.includes(:promotion_rules).distinct

        Rails.application.config.spree.promotions.rules.each do |rule_class|
          next unless rule_class.respond_to? :excluded_promotions_for_order
          scope = scope.where.not(id: rule_class.excluded_promotions_for_order(order).select(:id))
        end

        scope
      end

[incorvia]

This is great. Is this something that can be applied to Solidus core as a general technique? @elia if not, you are simply implementing this as an override of the sale_promotions?

[elia]

I think this can be generalized to Solidus core, maybe initially controlled by a configuration flag, but then be the default.

[softr8]

We have a similar performance issue with this escenario: Some users might get a welcome promo code that automatically applied to their first order

This caused a lot of promotions with promotion rules: first order, specific to one user, and potentially tied to a some taxons, and the action was to create adjustments in different percentages

So, Spree::PromotionHandler#sale_promotions would return all of them then it tried to activate every single promo, including other users promos that will never be activated

Our solution was to filter promotions that apply automatically that had the rule User and the value was not the user's email. This fixed the performance issue

[elia]

This is exactly the problem we faced as well and our solution was the same, nice to see convergence here!

[kennyadsl]

I think there's enough feedback to justify the need for this in Solidus Core. If we have this behavior enabled with a feature flag, well, that's an easier choice. I will bring this to our Core Team meeting and shoot an update here.

[kennyadsl]

@elia everyone in the Core Team is onboard with this direction, we are just waiting for the PR to review. Thanks!

[5minpause]

@kennyadsl may I ask which PR you are referring to? I am wondering about the state of these changes as they might impact my client's project positively. 😄

[kennyadsl]

@5minpause I think there's no PR yet. I was saying that if anyone is able to provide a PR, the Core Team is ok with the direction proposed in the discussion.

[5minpause]

FYI this topic is something that might affect my current client and could help them tremendously. As a result I will look into this. If it turns out that my client is affected by these changes I will try to open a PR for discussion.
For personal reason I will not be able to start work at all on this topic for at least 1-2 weeks.

[mamhoff]

#4304 is related, and will help future improvements in this domain: This (and other, more impactful) optimizations can only happen if the match policy for promotions is all, and one can quite easily get most of the benefit of an any match policy by duplicating the promotion.