maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Allowing VariantQuery for Feed Generation (#17382)

Browse source 
* Allowing VariantQuery for Feed Generation

Apologies for the breadth of this pull request, I had considered many
small commits, but felt that would've been more effort for the value
provided.

This commit includes the following:

- Documentation updates to the feed variant (though not the final pass)
- Renaming and adding RelevancyLevers that help differentiate
- Adding RelevancyLever#range to provide documentation
- Reducing redundent controller logic by making a
  `Articles::Feeds.feed_for` method.
- Adding some configuration validation for RelevancyLevers
- Adding constants for better clarification
- Testing unhappy paths for feed configuration
- Adjusting the module namespace of some objects
- Exposing top-level configurations for variants (along with their
  defaults)
- Creating the VariantyQuery that at present inherits from the
  `Articles::Feeds::WeightedQueryStrategy`

As implemented, we can deploy this code to production without using the
new VariantQuery.  Once we toggle on the
`:feed_uses_variant_query_feature` FeatureFlag, it will switch to using
the VariantQuery.  The VariantQuery's two variants and the
internal configuration of `Articles::Feeds::WeightedQueryStrategy`
produce the same query.

The goal of this factor is to allow for a quick on and off toggle of the
feed query; to ensure that what we introduce remains performant.

- Closes forem/forem#17272
- Closes forem/forem#17276
- Closes forem/forem#17216

In addition, I will be recording a code-walkthrough and linking that
recording to the pull request.

* Apply suggestions from code review

Co-authored-by: Mac Siri <krairit.siri@gmail.com>

Co-authored-by: Mac Siri <krairit.siri@gmail.com>
This commit is contained in:
Jeremy Friesen 2022-04-21 11:07:09 -04:00 • committed by GitHub
parent 2f4f98462e
commit 0d0464be2f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
20 changed files with 709 additions and 134 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
app/controllers/stories/feeds_controller.rb
View file

@ -38,17 +38,12 @@ module Stories
feed = if Settings::UserExperience.feed_strategy == "basic"
Articles::Feeds::Basic.new(user: current_user, page: @page, tag: params[:tag])
else
strategy = AbExperiment.get(
experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT,
controller: self, user: current_user,
default_value: AbExperiment::ORIGINAL_VARIANT
)
Articles::Feeds::WeightedQueryStrategy.new(
Articles::Feeds.feed_for(
user: current_user,
number_of_articles: 25,
controller: self,
page: @page,
tags: params[:tag],
strategy: strategy,
tag: params[:tag],
number_of_articles: 25,
)
end
Datadog.tracer.trace("feed.query",
@ -65,15 +60,16 @@ module Stories
end
def signed_out_base_feed
strategy = AbExperiment.get(experiment: AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT, controller: self,
user: current_user, default_value: AbExperiment::ORIGINAL_VARIANT)
feed = if strategy.weighted_query_strategy?
Articles::Feeds::WeightedQueryStrategy.new(user: current_user, page: @page, tags: params[:tag])
elsif Settings::UserExperience.feed_strategy == "basic"
# I'm a bit uncertain why we're skipping the user on this call.
feed = if Settings::UserExperience.feed_strategy == "basic"
Articles::Feeds::Basic.new(user: nil, page: @page, tag: params[:tag])
else
Articles::Feeds::LargeForemExperimental.new(user: current_user, page: @page, tag: params[:tag])
Articles::Feeds.feed_for(
user: current_user,
controller: self,
page: @page,
tag: params[:tag],
number_of_articles: 25,
)
end
Datadog.tracer.trace("feed.query",
span_type: "db",

22
app/models/ab_experiment.rb
View file

@ -11,10 +11,11 @@ class AbExperiment < SimpleDelegator
# public
private_class_method :new
# @see ./config/feed-variants/README.md
ORIGINAL_VARIANT = "original".freeze
CURRENT_FEED_STRATEGY_EXPERIMENT = FieldTest.config["experiments"]&.keys
&.detect { |e| e.start_with? "feed_strategy" }.freeze
&.detect { |e| e.start_with? "feed_strategy" }.freeze
# Sometimes we might want to repurpose the same AbExperiment logic
# for different experiments. This provides the tooling for that
@ -34,6 +35,23 @@ class AbExperiment < SimpleDelegator
EXPERIMENT_TO_METHOD_NAME_MAP.fetch(experiment, experiment)
end
# A convenience method for retrieving the feed variant
# @param controller [ApplicationController] the request context of
# the experiment.
# @param user [User] who are we running the experiment with
#
# @return [Object] a variant that the experimenter should know what it wants
#
# @see .get
def self.get_feed_variant_for(controller:, user:)
get(
controller: controller,
user: user,
default_value: ORIGINAL_VARIANT,
experiment: CURRENT_FEED_STRATEGY_EXPERIMENT,
)
end
def self.variants_for_experiment(experiment)
configured_experiment = FieldTest.config["experiments"][experiment]
configured_experiment["variants"] if configured_experiment
@ -64,7 +82,7 @@ class AbExperiment < SimpleDelegator
# on a configured set of values. They know which one they
# likely want. Let them give us a hint.
#
# @return [Object] the experimenter should know what it wants
# @return [Object] a variant that the experimenter should know what it wants
#
# @see config/field_test.yml file for configured experiments.
#

131
app/models/articles/feeds.rb
View file

@ -14,6 +14,10 @@ module Articles
# setting.
NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS = 18
DEFAULT_USER_EXPERIENCE_LEVEL = 5
DEFAULT_NEGATIVE_REACTION_THRESHOLD = -10
DEFAULT_POSITIVE_REACTION_THRESHOLD = 10
# @api private
#
# This method helps answer the question: What are the articles
@ -35,7 +39,7 @@ module Articles
#
# @note the days_since_published is something carried
# over from the LargeForemExperimental and may not be
# relevant given that we have the :daily_factor_decay.
# relevant given that we have the :daily_decay.
# However, this further limitation based on a user's
# second most recent page view helps further winnow down
# the result set.
@ -46,6 +50,40 @@ module Articles
time_of_second_latest_page_view - NUMBER_OF_HOURS_TO_OFFSET_USERS_LATEST_ARTICLE_VIEWS.hours
end
# Get the properly configured feed for the given user (and other parameters).
#
# @param controller [ApplicationController] used to retrieve the field_test variant
# @param user [User, NilClass] used to retrieve the variant and how we query the articles
# @param number_of_articles [Integer] the pagination page size
# @param page [Integer] the page on which to start pagination
# @param tag [NilClass, String] not used but carried forward for interface conformance
#
# @return [Articles::Feeds::VariantQuery] if the :feed_uses_variant_query_feature feature is
# enabled.
# @return [Articles::Feeds::WeightedQueryStrategy] if the :feed_uses_variant_query_feature
# feature is not enabled.
def self.feed_for(controller:, user:, number_of_articles:, page:, tag:)
variant = AbExperiment.get_feed_variant_for(controller: controller, user: user)
if FeatureFlag.enabled?(:feed_uses_variant_query_feature)
VariantQuery.build_for(
variant: variant,
user: user,
number_of_articles: number_of_articles,
page: page,
tag: tag,
)
else
WeightedQueryStrategy.new(
variant: variant,
user: user,
number_of_articles: number_of_articles,
page: page,
tag: tag,
)
end
end
# The available feed levers for this Forem instance.
#
# @return [Articles::Feeds::LeverCatalogBuilder]
@ -56,7 +94,7 @@ module Articles
# rubocop:disable Metrics/BlockLength
# The available levers for this forem instance.
LEVER_CATALOG = LeverCatalogBuilder.new do
order_by_lever(OrderByLever::DEFAULT_KEY,
order_by_lever(:relevancy_score_and_publication_date,
label: "Order by highest calculated relevancy score then latest published at time.",
order_by_fragment: "article_relevancies.relevancy_score DESC, articles.published_at DESC")
@ -67,14 +105,15 @@ module Articles
relevancy_lever(:comments_count_by_those_followed,
label: "Weight to give for the number of comments on the article from other users" \
"that the given user follows.",
range: "[0..∞)",
user_required: true,
select_fragment: "COUNT(comments_by_followed.id)",
joins_fragment: ["LEFT OUTER JOIN follows AS followed_user
joins_fragments: ["LEFT OUTER JOIN follows AS followed_user
ON articles.user_id = followed_user.followable_id
AND followed_user.followable_type = 'User'
AND followed_user.follower_id = :user_id
AND followed_user.follower_type = 'User'",
"LEFT OUTER JOIN comments AS comments_by_followed
"LEFT OUTER JOIN comments AS comments_by_followed
ON comments_by_followed.commentable_id = articles.id
AND comments_by_followed.commentable_type = 'Article'
AND followed_user.followable_id = comments_by_followed.user_id
@ -84,12 +123,14 @@ module Articles
relevancy_lever(:comments_count,
label: "Weight to give to the number of comments on the article.",
range: "[0..∞)",
user_required: false,
select_fragment: "articles.comments_count",
group_by_fragment: "articles.comments_count")
relevancy_lever(:daily_decay,
label: "Weight given based on the relative age of the article",
range: "[0..∞)",
user_required: true,
select_fragment: "(current_date - articles.published_at::date)",
group_by_fragment: "articles.published_at")
@ -97,6 +138,7 @@ module Articles
relevancy_lever(:experience,
label: "Weight to give based on the difference between experience level of the " \
"article and given user.",
range: "[0..∞)",
user_required: true,
select_fragment: "ROUND(ABS(articles.experience_level_rating - (SELECT
(CASE
@ -108,15 +150,17 @@ module Articles
relevancy_lever(:featured_article,
label: "Weight to give for feature or unfeatured articles. 1 is featured.",
user_required: false,
range: "[0..1]",
select_fragment: "(CASE articles.featured WHEN true THEN 1 ELSE 0 END)",
group_by_fragment: "articles.featured")
relevancy_lever(:following_author,
label: "Weight to give when the given user follows the article's author." \
"1 is followed, 0 is not followed.",
range: "[0..1]",
user_required: true,
select_fragment: "COUNT(followed_user.follower_id)",
joins_fragment: ["LEFT OUTER JOIN follows AS followed_user
joins_fragments: ["LEFT OUTER JOIN follows AS followed_user
ON articles.user_id = followed_user.followable_id
AND followed_user.followable_type = 'User'
AND followed_user.follower_id = :user_id
@ -125,9 +169,10 @@ module Articles
relevancy_lever(:following_org,
label: "Weight to give to the when the given user follows the article's organization." \
"1 is followed, 0 is not followed.",
range: "[0..1]",
user_required: true,
select_fragment: "COUNT(followed_org.follower_id)",
joins_fragment: ["LEFT OUTER JOIN follows AS followed_org
joins_fragments: ["LEFT OUTER JOIN follows AS followed_org
ON articles.organization_id = followed_org.followable_id
AND followed_org.followable_type = 'Organization'
AND followed_org.follower_id = :user_id
@ -135,25 +180,81 @@ module Articles
relevancy_lever(:latest_comment,
label: "Weight to give an article based on it's most recent comment.",
range: "[0..∞)",
user_required: false,
select_fragment: "(current_date - MAX(comments.created_at)::date)",
joins_fragment: ["LEFT OUTER JOIN comments
joins_fragments: ["LEFT OUTER JOIN comments
ON comments.commentable_id = articles.id
AND comments.commentable_type = 'Article'
AND comments.deleted = false
AND comments.created_at > :oldest_published_at"])
relevancy_lever(:matching_tags,
label: "Weight to give for the sum points of the intersecting tags of the article" \
"user positive follows.",
relevancy_lever(:matching_negative_tag_intersection_count,
label: "Weight to give the number of intersecting tags of the article and " \
"user negative follows",
range: "[0..4]",
user_required: true,
select_fragment: "LEAST(10.0, SUM(followed_tags.points))::integer",
joins_fragment: ["LEFT OUTER JOIN taggings
select_fragment: "COUNT(negative_followed_tags.id)",
joins_fragments: ["LEFT OUTER JOIN taggings
ON taggings.taggable_id = articles.id
AND taggable_type = 'Article'",
"INNER JOIN tags
"INNER JOIN tags
ON taggings.tag_id = tags.id",
"LEFT OUTER JOIN follows AS followed_tags
"LEFT OUTER JOIN follows AS negative_followed_tags
ON tags.id = negative_followed_tags.followable_id
AND negative_followed_tags.followable_type = 'ActsAsTaggableOn::Tag'
AND negative_followed_tags.follower_type = 'User'
AND negative_followed_tags.follower_id = :user_id
AND negative_followed_tags.explicit_points < 0"])
relevancy_lever(:matching_negative_tags_intersection_points,
label: "Weight to give for the sum points of the intersecting tags of the article and " \
"user positive follows.",
user_required: true,
range: "[-10..0]",
select_fragment: "LEAST(-10.0, SUM(followed_tags.points))::integer",
joins_fragments: ["LEFT OUTER JOIN taggings
ON taggings.taggable_id = articles.id
AND taggable_type = 'Article'",
"INNER JOIN tags
ON taggings.tag_id = tags.id",
"LEFT OUTER JOIN follows AS followed_tags
ON tags.id = followed_tags.followable_id
AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag'
AND followed_tags.follower_type = 'User'
AND followed_tags.follower_id = :user_id
AND followed_tags.explicit_points < 0"])
relevancy_lever(:matching_positive_tags_intersection_count,
label: "Weight to give for number of the intersecting tags of the article and " \
"user positive follows.",
range: "[0..4]",
user_required: true,
select_fragment: "COUNT(followed_tags.id)",
joins_fragments: ["LEFT OUTER JOIN taggings
ON taggings.taggable_id = articles.id
AND taggable_type = 'Article'",
"INNER JOIN tags
ON taggings.tag_id = tags.id",
"LEFT OUTER JOIN follows AS followed_tags
ON tags.id = followed_tags.followable_id
AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag'
AND followed_tags.follower_type = 'User'
AND followed_tags.follower_id = :user_id
AND followed_tags.explicit_points >= 0"])
relevancy_lever(:matching_positive_tags_intersection_points,
label: "Weight to give for the sum points of the intersecting tags of the article and " \
"user positive follows.",
user_required: true,
range: "[0..10]",
select_fragment: "LEAST(10.0, SUM(followed_tags.points))::integer",
joins_fragments: ["LEFT OUTER JOIN taggings
ON taggings.taggable_id = articles.id
AND taggable_type = 'Article'",
"INNER JOIN tags
ON taggings.tag_id = tags.id",
"LEFT OUTER JOIN follows AS followed_tags
ON tags.id = followed_tags.followable_id
AND followed_tags.followable_type = 'ActsAsTaggableOn::Tag'
AND followed_tags.follower_type = 'User'
@ -163,6 +264,7 @@ module Articles
relevancy_lever(:privileged_user_reaction,
label: "-1 when privileged user reactions down-vote, 0 when netural, and 1 when positive.",
user_required: false,
range: "[-1..1]",
select_fragment: "(CASE
WHEN articles.privileged_users_reaction_points_sum < :negative_reaction_threshold THEN -1
WHEN articles.privileged_users_reaction_points_sum > :positive_reaction_threshold THEN 1
@ -171,6 +273,7 @@ module Articles
relevancy_lever(:public_reactions,
label: "Weight to give for the number of unicorn, heart, reading list reactions for article.",
range: "[0..∞)",
user_required: false,
select_fragment: "articles.public_reactions_count",
group_by_fragment: "articles.public_reactions_count")

15
app/models/articles/feeds/lever_catalog_builder.rb
View file

@ -9,15 +9,10 @@ module Articles
end
end
class ConfigurationError < StandardError
end
# @yieldparam [Articles::Feeds::LeverCatalogBuilder]
#
# @raise [Articles::Feeds::LeverCatalogBuilder::DuplicateLeverError] when you attempt to
# register a lever with the same key.
# @raise [Articles::Feeds::LeverCatalogBuilder::ConfigurationError] when you fail to configure
# the default order_by_lever
#
# @note Once initialized, this object and its constituent parts are frozen to prevent further
# modification. In other words, once instantiated our published catalog has been
@ -30,8 +25,6 @@ module Articles
# `#instance_exec` method.
instance_exec(&config)
raise ConfigurationError unless @order_by_levers.key?(OrderByLever::DEFAULT_KEY)
@relevancy_levers.freeze
@order_by_levers.freeze
freeze
@ -46,17 +39,13 @@ module Articles
@relevancy_levers.fetch(key.to_sym)
end
# @param key [#to_sym, NilClass] when given nil fallback to the OrderByLever::DEFAULT_KEY.
# @param key [#to_sym]
# @return [Articles::Feeds::OrderByLever]
#
# @raise [KeyError] if the given key is not found in the list of sort levers; in other words
# we have a configuration mismatch.
#
# @see Articles::Feeds::OrderByLever::DEFAULT_KEY
def fetch_order_by(key)
key ||= OrderByLever::DEFAULT_KEY
key = key.to_sym
@order_by_levers.fetch(key)
@order_by_levers.fetch(key.to_sym)
end
protected

6
app/models/articles/feeds/order_by_lever.rb
View file

@ -5,8 +5,6 @@ module Articles
#
# @see config/feed/README.md
class OrderByLever
DEFAULT_KEY = :relevancy_score_and_publication_date
# @param key [Symbol] the programmatic means of naming this
# lever. (e.g. "publication_date_decay_lever")
# @param label [String] the "help text" for describing this lever. (e.g. "How the
@ -18,6 +16,10 @@ module Articles
@order_by_fragment = order_by_fragment
end
attr_reader :key, :label, :order_by_fragment
def to_sql
Arel.sql(order_by_fragment)
end
end
end
end

75
app/models/articles/feeds/relevancy_lever.rb
View file

@ -5,29 +5,96 @@ module Articles
#
# @see config/feed/README.md
class RelevancyLever
class ConfigurationError < StandardError
end
class InvalidFallbackError < ConfigurationError
def initialize(fallback:, key:)
super("Expected fallback to be a Numeric value for lever #{key.inspect}, got #{fallback.inspect}")
end
end
class InvalidCasesError < ConfigurationError
def initialize(cases:, key:)
super("Expected cases to be an array of number pairs for lever #{key.inspect}, got #{cases.inspect}")
end
end
Configured = Struct.new(
:key,
:user_required,
:select_fragment,
:joins_fragments,
:group_by_fragment,
:cases,
:fallback,
keyword_init: true,
) do
alias_method :user_required?, :user_required
end
# @param key [Symbol] the programmatic means of naming this
# lever. (e.g. "publication_date_decay_lever")
# @param label [String] the the "help text" for describing this lever. (e.g. "How the
# publication date impacts relevancy score?")
# @param range [String] the expected range of the query results.
# @param user_required [Boolean] if true, this lever is only available when we are building
# the feed query for a given user.
# @param select_fragment [String] a SQL `SELECT` fragment used to create the *lever range*
# @param joins_fragment [Array<String>] an array of SQL `JOIN` fragments used to ensure the
# @param joins_fragments [Array<String>] an array of SQL `JOIN` fragments used to ensure the
# given :select_fragment can properly query the database.
# @param group_by_fragment [String] a SQL `GROUP BY` fragment used to ensure the given
# :select_fragment can properly query the database.
def initialize(key:, label:, user_required:, select_fragment:, joins_fragment: [], group_by_fragment: nil)
# rubocop:disable Layout/LineLength
def initialize(key:, label:, range:, user_required:, select_fragment:, joins_fragments: [], group_by_fragment: nil)
@key = key.to_sym
@label = label
@range = range
@user_required = user_required
@select_fragment = select_fragment
@joins_fragment = Array.wrap(joins_fragment)
@joins_fragments = Array.wrap(joins_fragments)
@group_by_fragment = group_by_fragment
end
# rubocop:enable Layout/LineLength
attr_reader :key, :label, :user_required, :select_fragment, :joins_fragment, :group_by_fragment
attr_reader :key, :label, :user_required, :select_fragment, :joins_fragments, :group_by_fragment
alias user_required? user_required
# Responsible for configuring the lever with the given input.
#
# @param cases [Array<Array<Integer, Float>>]
# @param fallback [Float]
#
# @return [Articles::Feeds::RelevancyLever::Configured]
# @raise [Articles::Feeds::RelevancyLever::InvalidFallbackError] when the given fallback is
# invalid.
# @raise [Articles::Feeds::RelevancyLever::InvaidCasesError] when the given cases is invalid.
def configure_with(cases:, fallback:)
raise InvalidFallbackError.new(fallback: fallback, key: key) unless valid_fallback?(fallback)
raise InvalidCasesError.new(cases: cases, key: key) unless valid_cases?(cases)
Configured.new(
key: key,
user_required: user_required,
select_fragment: select_fragment,
joins_fragments: joins_fragments,
group_by_fragment: group_by_fragment,
cases: cases,
fallback: fallback,
)
end
private
def valid_fallback?(fallback)
fallback.is_a?(Numeric)
end
def valid_cases?(cases)
return false unless cases.is_a?(Array)
cases.all? { |range, factor| range.is_a?(Numeric) && factor.is_a?(Numeric) }
end
end
end
end

73
app/models/articles/feeds/variant_assembler.rb
View file

@ -5,68 +5,63 @@ module Articles
#
# @see .call
module VariantAssembler
# The Rails root relative path to the directory of feed variants
DIRECTORY = "config/feed-variants".freeze
# The default extension for feed variants
EXTENSION = "json".freeze
# Assemble the named :variant based on the configuration of levers.
#
# @param variant [#to_sym,String,Symbol] the name of the variant we're assembling
# @param levers [Articles::Feeds::LeverCatalogBuilder] the available levers for assembly
# @param variants [Hash] the cache of previously assembled variants
# @param catalog [Articles::Feeds::LeverCatalogBuilder] the available levers for assembly
# @param variants [Hash] the cache of previously assembled variants; don't go rebuilding if we
# already have one.
# @param dir [String] the relative directory that contains the variants.
#
# @return [Articles::Feeds::VariantQueryConfig]
def self.call(variant:, levers: Articles::Feeds.lever_catalog, variants: variants_cache)
# @raise [Errno::ENOENT] if named variant does not exist in DIRECTORY. In other
# words, we have a mismatch in configuration.
#
# @return [Articles::Feeds::VariantQuery::Config]
def self.call(variant:, catalog: Articles::Feeds.lever_catalog, variants: variants_cache, dir: DIRECTORY)
variant = variant.to_sym
variants[variant] ||= begin
content = Rails.root.join("config/feed-variants/#{variant}.json").read
content = Rails.root.join(dir, "#{variant}.#{EXTENSION}").read
config = JSON.parse(content)
build_with(levers: levers, config: config, variant: variant)
build_with(catalog: catalog, config: config, variant: variant)
end
end
# @return [Hash<Symbol, VariantQuery::Config>]
def self.variants_cache
@variants_cache ||= {}
end
private_class_method :variants_cache
def self.build_with(levers:, config:, variant:)
# @param catalog [Articles::Feeds::LeverCatalogBuilder]
# @param variant [Symbol]
# @param config [Hash]
#
# @raise [KeyError] if we he have an invalidate configuration.
#
# @return [Articles::Feeds::VariantQuery::Config]
def self.build_with(catalog:, config:, variant:)
relevancy_levers = config.fetch("levers").map do |key, settings|
lever = levers.fetch_lever(key)
ConfiguredLever.new(
key: lever.key,
user_required: lever.user_required,
select_fragment: lever.select_fragment,
joins_fragment: lever.joins_fragment,
group_by_fragment: lever.group_by_fragment,
cases: settings.fetch("cases"),
fallback: settings.fetch("fallback"),
)
lever = catalog.fetch_lever(key)
lever.configure_with(cases: settings.fetch("cases"), fallback: settings.fetch("fallback"))
end
VariantQueryConfig.new(
VariantQuery::Config.new(
variant: variant,
levers: relevancy_levers,
order_by: levers.fetch_order_by(config["order_by"]),
order_by: catalog.fetch_order_by(config.fetch("order_by")),
max_days_since_published: config.fetch("max_days_since_published"),
default_user_experience_level: config.fetch("default_user_experience_level", DEFAULT_USER_EXPERIENCE_LEVEL),
negative_reaction_threshold: config.fetch("negative_reaction_threshold", DEFAULT_NEGATIVE_REACTION_THRESHOLD),
positive_reaction_threshold: config.fetch("positive_reaction_threshold", DEFAULT_POSITIVE_REACTION_THRESHOLD),
)
end
private_class_method :build_with
end
VariantQueryConfig = Struct.new(
:variant,
:levers,
:order_by,
keyword_init: true,
)
ConfiguredLever = Struct.new(
:key,
:user_required,
:select_fragment,
:joins_fragment,
:group_by_fragment,
:cases,
:fallback,
keyword_init: true,
) do
alias_method :user_required?, :user_required
end
end
end

113
app/services/articles/feeds/variant_query.rb Normal file
View file

@ -0,0 +1,113 @@
module Articles
module Feeds
# @note In the current implementation this is inheriting from WeightedQueryStrategy; going
# forward, we want to move away from the less flexible WeightedQueryStrategy. However, as
# we roll this out, we want both to be utilized. In part so we can have a quick fallback
# to a known working state (e.g. WeightedQueryStrategy). Assuming we move forward with
# this implementation, we will break the inheritance, copy the relevant methods over, and
# remove the WeightedQueryStrategy.
class VariantQuery < WeightedQueryStrategy
# @param variant [Symbol, #to_sym] the name of the variant query we're building.
# @param assembler [Articles::Feeds::VariantAssembler, #call] responsible for converting the
# given variant to a config suitable for building a VariantQuery.
# @param kwargs [Hash] named parameters to pass along to the #initialize method.
#
# @return [Articles::Feeds::VariantQuery]
#
# @see #initialize
def self.build_for(variant:, assembler: VariantAssembler, **kwargs)
config = assembler.call(variant: variant)
new(config: config, **kwargs)
end
# Let's make sure that folks initialize this with a variant configuration.
private_class_method :new
Config = Struct.new(
:variant,
:levers, # Array <Articles::Feeds::RelevancyLever::Configured>
:order_by, # Articles::Feeds::OrderByLever
:max_days_since_published,
:default_user_experience_level,
:negative_reaction_threshold,
:positive_reaction_threshold,
keyword_init: true,
)
# @param config [Articles::Feeds::VariantQuery::Config]
# @param user [User,NilClass]
# @param number_of_articles [Integer, #to_i]
# @param page [Integer, #to_i]
# @param tag [NilClass] not used
#
# rubocop:disable Lint/MissingSuper
def initialize(config:, user: nil, number_of_articles: 50, page: 1, tag: nil)
@user = user
@number_of_articles = number_of_articles
@page = page
@tag = tag
@config = config
@oldest_published_at = Articles::Feeds.oldest_published_at_to_consider_for(
user: @user,
days_since_published: max_days_since_published,
)
configure!
end
# rubocop:enable Lint/MissingSuper
delegate(
:max_days_since_published,
:negative_reaction_threshold,
:positive_reaction_threshold,
:default_user_experience_level,
to: :config,
)
attr_reader :config
private
def final_order_logic(articles)
articles.order(config.order_by.to_sql)
end
def configure!
@relevance_score_components = []
# By default we always need to group by the articles.id
# column. And as we add scoring methods to the query, we need
# to add additional group_by clauses based on the chosen
# scoring method.
@group_by_fields = Set.new
@group_by_fields << "articles.id"
@joins = Set.new
# Ensure that we honor a user's block requests.
unless @user.nil?
@joins << "LEFT OUTER JOIN user_blocks
ON user_blocks.blocked_id = articles.user_id
AND user_blocks.blocked_id IS NULL
AND user_blocks.blocker_id = :user_id"
end
config.levers.each do |lever|
# Don't attempt to use this factor if we don't have user.
next if lever.user_required? && @user.nil?
# This scoring method requires a group by clause.
@group_by_fields << lever.group_by_fragment if lever.group_by_fragment.present?
@joins += lever.joins_fragments if lever.joins_fragments.present?
@relevance_score_components << build_score_element_from(
clause: lever.select_fragment,
cases: lever.cases,
fallback: lever.fallback,
)
end
end
end
end
end

30
app/services/articles/feeds/weighted_query_strategy.rb
View file

@ -230,18 +230,13 @@ module Articles
}
}.freeze
DEFAULT_USER_EXPERIENCE_LEVEL = 5
DEFAULT_NEGATIVE_REACTION_THRESHOLD = -10
DEFAULT_POSITIVE_REACTION_THRESHOLD = 10
# @param user [User] who are we querying for?
# @param number_of_articles [Integer] how many articles are we
# returning
# @param page [Integer] what is the pagination page
# @param tag [String, nil] this isn't implemented in other feeds
# so we'll see
# @param strategy [String, "original"] pass a current a/b test in
# @param variant [String, "original"] pass a current a/b test in
# @param config [Hash<Symbol, Object>] a list of configurations,
# see {#initialize} implementation details.
# @option config [Array<Symbol>] :scoring_configs
@ -260,13 +255,13 @@ module Articles
# those will likely need some kind of structured consideration.
#
# rubocop:disable Layout/LineLength
def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil, strategy: AbExperiment::ORIGINAL_VARIANT, **config)
def initialize(user: nil, number_of_articles: 50, page: 1, tag: nil, variant: AbExperiment::ORIGINAL_VARIANT, **config)
@user = user
@number_of_articles = number_of_articles.to_i
@page = (page || 1).to_i
# TODO: The tag parameter is vestigial, there's no logic around this value.
@tag = tag
@strategy = strategy
@variant = variant
@default_user_experience_level = config.fetch(:default_user_experience_level) { DEFAULT_USER_EXPERIENCE_LEVEL }
@negative_reaction_threshold = config.fetch(:negative_reaction_threshold, DEFAULT_NEGATIVE_REACTION_THRESHOLD)
@positive_reaction_threshold = config.fetch(:positive_reaction_threshold, DEFAULT_POSITIVE_REACTION_THRESHOLD)
@ -278,6 +273,8 @@ module Articles
days_since_published: @days_since_published,
)
end
attr_reader :oldest_published_at, :positive_reaction_threshold, :negative_reaction_threshold, :default_user_experience_level
# rubocop:enable Layout/LineLength
# The goal of this query is to generate a list of articles that
@ -316,9 +313,9 @@ module Articles
# rubocop:disable Layout/LineLength
def call(only_featured: false, must_have_main_image: false, limit: default_limit, offset: default_offset, omit_article_ids: [])
repeated_query_variables = {
negative_reaction_threshold: @negative_reaction_threshold,
positive_reaction_threshold: @positive_reaction_threshold,
oldest_published_at: @oldest_published_at,
negative_reaction_threshold: negative_reaction_threshold,
positive_reaction_threshold: positive_reaction_threshold,
oldest_published_at: oldest_published_at,
omit_article_ids: omit_article_ids,
now: Time.current
}
@ -344,7 +341,7 @@ module Articles
),
repeated_query_variables.merge({
user_id: @user.id,
default_user_experience_level: @default_user_experience_level.to_i
default_user_experience_level: default_user_experience_level.to_i
}),
]
end
@ -359,10 +356,11 @@ module Articles
# can use to help ensure that we can use all of the
# ActiveRecord goodness of scopes (e.g.,
# limited_column_select) and eager includes.
Article.joins(join_fragment)
results = Article.joins(join_fragment)
.limited_column_select
.includes(top_comments: :user)
.order("article_relevancies.relevancy_score DESC, articles.published_at DESC")
final_order_logic(results)
end
# rubocop:enable Layout/LineLength
@ -438,6 +436,10 @@ module Articles
private
def final_order_logic(scope)
scope.order("article_relevancies.relevancy_score DESC, articles.published_at DESC")
end
# Concatenate the required group by clauses.
#
# @return [String]

4
config/feed-variants/20220415-incumbent.json
View file

@ -1,4 +1,6 @@
{
"max_days_since_published": 15,
"order_by": "relevancy_score_and_publication_date",
"levers": {
"daily_decay": {
"cases": [
@ -65,7 +67,7 @@
],
"fallback": 0.988
},
"matching_tags": {
"matching_positive_tags_intersection_points": {
"cases": [
[0, 0.7],
[1, 0.7303],

50
config/feed-variants/README.md Normal file
View file

@ -0,0 +1,50 @@
# Feed Variants
This folder contains the feed variants that we have, are, or will be testing. By
convention the file's basename without extension is the name of the variant
(e.g. "./config/feed-variants/20220415-incumbent.json" encodes the variant named
`20220415-incumbent`).
There exists one variant, `original.json`, which is our fallback variant. This
fallback is performant and adequate for generating the feed for DEV.to. It is
present in case we need to quickly turn off an experiment. The name `original`
is the same as the `AbExperiment::ORIGINAL_VARIANT` constant.
**Do not remove `original.json` without consideration for how to rollback a feed
experiment.**
Variants are loaded into production as needed. That "need" is determined by the
configuration of the `./config/field_test.yml`. Once we load a variant into
production, we cache that variant (see
`Articles::Feeds::VariantAssembler.variants_cache`).
As part of our test suite we assemble and verify each defined variant.
## How to Make a Feed Variant
A feed variant is written in JSON format. A feed variant configures one or more
`Articles::Feeds::RelevancyLevers`. The variant configuration defines:
- The cases and fallback for each of the selected relevancy levers
- Optionally the sort order of the query results
- Optionally a few high level parameters.
The relevancy levers and their SQL query fragments are defined in
`Articles::Feeds::LEVER_CATALOG`. Any levers not configured for a variant are
omitted for that variant (but available for other variants).
The available sort order is also defined in `Articles::Feeds::LEVER_CATALOG`.
The high level parameters are defined in `Aritcles::Feeds::VariantQuery::Config`
and as of <2022-04-20 Wed> are:
- **_max_days_since_published_:** only consider articles that were published no
more than the _max_days_since_published_.
- **_default_user_experience_level_:** what do we consider to be the default
user experience level.
- **_negative_reaction_threshold_:** if an article has less than this value for
it's trusted/privileged user reaction points, consider it a low quality
article.
- **_positive_reaction_threshold_:** if an article has more than this value for
it's trusted/privileged user reaction points, consider it a low quality
article.

102
config/feed-variants/original.json Normal file
View file

@ -0,0 +1,102 @@
{
"max_days_since_published": 15,
"order_by": "relevancy_score_and_publication_date",
"levers": {
"daily_decay": {
"cases": [
[0, 1],
[1, 0.99],
[2, 0.985],
[3, 0.98],
[4, 0.975],
[5, 0.97],
[6, 0.965],
[7, 0.96],
[8, 0.955],
[9, 0.95],
[10, 0.945],
[11, 0.94],
[12, 0.935],
[13, 0.93],
[14, 0.925]
],
"fallback": 0.9
},
"comments_count_by_those_followed": {
"cases": [
[0, 0.95],
[1, 0.98],
[2, 0.99]
],
"fallback": 0.93
},
"comments_count": {
"cases": [
[0, 0.8],
[1, 0.82],
[2, 0.84],
[3, 0.86],
[4, 0.88],
[5, 0.9],
[6, 0.92],
[7, 0.94],
[8, 0.96],
[9, 0.98]
],
"fallback": 1
},
"featured_article": { "cases": [[1, 1]], "fallback": 0.85 },
"following_author": {
"cases": [
[0, 0.8],
[1, 1]
],
"fallback": 1
},
"following_org": {
"cases": [
[0, 0.95],
[1, 1]
],
"fallback": 1
},
"latest_comment": {
"cases": [
[0, 1],
[1, 0.9988]
],
"fallback": 0.988
},
"matching_positive_tags_intersection_points": {
"cases": [
[0, 0.7],
[1, 0.7303],
[2, 0.7606],
[3, 0.7909],
[4, 0.8212],
[5, 0.8515],
[6, 0.8818],
[7, 0.9121],
[8, 0.9424],
[9, 0.9727]
],
"fallback": 1
},
"privileged_user_reaction": {
"cases": [
[-1, 0.2],
[1, 1]
],
"fallback": 0.95
},
"public_reactions": {
"cases": [
[0, 0.9988],
[1, 0.9988],
[2, 0.9988],
[3, 0.9988]
],
"fallback": 1
}
}
}

26
spec/fixtures/feed-variants/broken/broken-fallback.json vendored Normal file
View file

@ -0,0 +1,26 @@
{
"max_days_since_published": 15,
"order_by": "relevancy_score_and_publication_date",
"levers": {
"daily_decay": {
"cases": [
[0, 1],
[1, 0.99],
[2, 0.985],
[3, 0.98],
[4, 0.975],
[5, 0.97],
[6, 0.965],
[7, 0.96],
[8, 0.955],
[9, 0.95],
[10, 0.945],
[11, 0.94],
[12, 0.935],
[13, 0.93],
[14, 0.925]
],
"fallback": "OR SELECT * FROM SOMETHING"
}
}
}

13
spec/models/ab_experiment_spec.rb
View file

@ -4,6 +4,19 @@ RSpec.describe AbExperiment do
let(:controller) { ApplicationController.new }
let(:user) { double }
describe ".get_feed_variant_for" do
before do
allow(controller).to receive(:field_test).with(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT,
participant: user).and_return("special")
end
it "returns an inquirable string" do
result = described_class.get_feed_variant_for(user: user, controller: controller)
expect(result).to eq("special")
expect(result).to be_special
end
end
describe ".get" do
before do
allow(controller).to receive(:field_test).with(AbExperiment::CURRENT_FEED_STRATEGY_EXPERIMENT,

22
spec/models/articles/feeds/lever_catalog_builder_spec.rb
View file

@ -3,18 +3,20 @@ require "rails_helper"
RSpec.describe Articles::Feeds::LeverCatalogBuilder do
let(:catalog) do
described_class.new do
order_by_lever(Articles::Feeds::OrderByLever::DEFAULT_KEY,
order_by_lever(:order_by_this,
label: "Hello world",
order_by_fragment: "relevancy_score DESC")
relevancy_lever(:my_key,
user_required: true,
label: "A label",
range: "[-10..0]",
select_fragment: "articles.count")
relevancy_lever(:my_other_key,
user_required: false,
label: "Another label",
range: "[-10..0]",
select_fragment: "articles.reactions_count")
end
end
@ -31,11 +33,13 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do
relevancy_lever(:my_key,
user_required: true,
label: "A label",
range: "[-10..0]",
select_fragment: "articles.count")
relevancy_lever(:my_key,
user_required: false,
label: "Another label",
range: "[-10..0]",
select_fragment: "articles.reactions_count")
end
end.to raise_error(described_class::DuplicateLeverError)
@ -57,17 +61,17 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do
end
describe "#fetch_lever" do
subject { catalog.fetch_lever(lever) }
subject { catalog.fetch_lever(key) }
context "when lever exists" do
let(:lever) { "my_key" }
let(:key) { "my_key" }
it { is_expected.to be_a(Articles::Feeds::RelevancyLever) }
it { is_expected.to be_frozen }
end
context "when lever does not exist" do
let(:lever) { "404" }
let(:key) { "404" }
it { within_block_is_expected.to raise_error(KeyError) }
end
@ -77,15 +81,7 @@ RSpec.describe Articles::Feeds::LeverCatalogBuilder do
subject { catalog.fetch_order_by(key) }
context "when lever exists" do
let(:key) { Articles::Feeds::OrderByLever::DEFAULT_KEY }
it { is_expected.to be_a(Articles::Feeds::OrderByLever) }
it { is_expected.to be_frozen }
end
context "when given a nil key" do
# Note this assumes that we've configured the feed correctly
let(:key) { nil }
let(:key) { "order_by_this" }
it { is_expected.to be_a(Articles::Feeds::OrderByLever) }
it { is_expected.to be_frozen }

1
spec/models/articles/feeds/order_by_lever_spec.rb
View file

@ -12,4 +12,5 @@ RSpec.describe Articles::Feeds::OrderByLever do
it { is_expected.to respond_to :key }
it { is_expected.to respond_to :label }
it { is_expected.to respond_to :order_by_fragment }
it { is_expected.to respond_to :to_sql }
end

32
spec/models/articles/feeds/relevancy_lever_spec.rb
View file

@ -1,9 +1,12 @@
require "rails_helper"
RSpec.describe Articles::Feeds::RelevancyLever do
subject do
subject { lever }
let(:lever) do
described_class.new(
key: :my_key,
range: "[0..10)",
label: "My label",
select_fragment: "articles.reaction_count",
user_required: true,
@ -13,8 +16,33 @@ RSpec.describe Articles::Feeds::RelevancyLever do
it { is_expected.to respond_to :key }
it { is_expected.to respond_to :label }
it { is_expected.to respond_to :select_fragment }
it { is_expected.to respond_to :joins_fragment }
it { is_expected.to respond_to :joins_fragments }
it { is_expected.to respond_to :group_by_fragment }
it { is_expected.to respond_to :user_required }
it { is_expected.to respond_to :user_required? }
describe "#configure_with" do
subject { lever.configure_with(fallback: fallback, cases: cases) }
let(:cases) { [[0, 0]] }
let(:fallback) { 1 }
context "when fallback is not a number" do
let(:fallback) { "nope" }
it { within_block_is_expected.to raise_error described_class::InvalidFallbackError }
end
context "when cases is not an array" do
let(:cases) { "nope" }
it { within_block_is_expected.to raise_error described_class::InvalidCasesError }
end
context "when cases includes a non-number" do
let(:cases) { [[0, 1], [1, "a"]] }
it { within_block_is_expected.to raise_error described_class::InvalidCasesError }
end
end
end

10
spec/models/articles/feeds/variant_assembler_spec.rb
View file

@ -2,14 +2,20 @@ require "rails_helper"
RSpec.describe Articles::Feeds::VariantAssembler do
describe ".call" do
Rails.root.glob("config/feed-variants/*.json").each do |pathname|
Rails.root.glob("#{described_class::DIRECTORY}/*.#{described_class::EXTENSION}").each do |pathname|
variant = pathname.basename(".json").to_s.to_sym
context "for #{variant.inspect}" do
# NOTE: We're providing the variants so as to not pollute the cache for other tests.
subject { described_class.call(variant: variant, variants: {}) }
it { is_expected.to be_a(Articles::Feeds::VariantQueryConfig) }
it { is_expected.to be_a(Articles::Feeds::VariantQuery::Config) }
end
end
context "with missing variant" do
subject { described_class.call(variant: :obviously_missing, variants: {}) }
it { within_block_is_expected.to raise_error(Errno::ENOENT) }
end
end
end

30
spec/requests/stories/feeds_spec.rb
View file

@ -46,20 +46,26 @@ RSpec.describe "Stories::Feeds", type: :request do
)
end
it "returns feed when feed_strategy is optimized" do
allow(Settings::UserExperience).to receive(:feed_strategy).and_return("optimized")
%i[enable disable].each do |toggle|
context "when we #{toggle} :feed_uses_variant_query_feature" do
before { FeatureFlag.public_send(toggle, :feed_uses_variant_query_feature) }
get stories_feed_path
it "returns feed when feed_strategy is not basic" do
allow(Settings::UserExperience).to receive(:feed_strategy).and_return("optimized")
expect(response_article).to include(
"id" => article.id,
"title" => title,
"user_id" => user.id,
"user" => hash_including("name" => user.name),
"organization_id" => organization.id,
"organization" => hash_including("name" => organization.name),
"tag_list" => article.decorate.cached_tag_list_array,
)
get stories_feed_path
expect(response_article).to include(
"id" => article.id,
"title" => title,
"user_id" => user.id,
"user" => hash_including("name" => user.name),
"organization_id" => organization.id,
"organization" => hash_including("name" => organization.name),
"tag_list" => article.decorate.cached_tag_list_array,
)
end
end
end
context "when rendering an article that is pinned" do

60
spec/services/articles/feeds/variant_query_spec.rb Normal file
View file

@ -0,0 +1,60 @@
require "rails_helper"
RSpec.describe Articles::Feeds::VariantQuery, type: :service do
# We're exercising each named feed variant to ensure that the queries are valid SQL.
describe ".build_for" do
Rails.root.glob(File.join(Articles::Feeds::VariantAssembler::DIRECTORY, "/*.json")).each do |pathname|
variant = pathname.basename(".json").to_s.to_sym
subject(:query_call) { variant_query.call }
let(:variant_query) { described_class.build_for(variant: variant, user: user) }
context "for #{variant.inspect} and user is nil" do
let(:user) { nil }
it "is a valid ActiveRecord::Relation" do
article = create(:article)
expect(query_call).to be_a(ActiveRecord::Relation)
expect(query_call.to_a).to match_array(article)
end
end
context "for #{variant.inspect} and a non-nil user" do
let(:user) { create(:user) }
it "is a valid ActiveRecord::Relation" do
article = create(:article)
expect(query_call).to be_a(ActiveRecord::Relation)
expect(query_call.to_a).to match_array(article)
end
end
end
Rails.root.glob("spec/fixtures/feed-variants/broken/*.json").each do |pathname|
variant = pathname.basename(".json").to_s.to_sym
let(:variant_config) do
# Assembling the config and not polluting the variant cache with broken levers.
Articles::Feeds::VariantAssembler.call(variant: variant, variants: {},
dir: "spec/fixtures/feed-variants/broken")
end
# We already assembled the variant's config, let's short circuit that for the query
let(:stubbed_assembler) { ->(*) { variant_config } }
context "for broken variant #{variant.inspect} and a non-nil user" do
let(:variant_query) { described_class.build_for(variant: variant, user: user, assembler: stubbed_assembler) }
let(:user) { create(:user) }
it { within_block_is_expected.to raise_error(Articles::Feeds::RelevancyLever::ConfigurationError) }
end
context "for broken variant #{variant.inspect} and a nil user" do
let(:variant_query) { described_class.build_for(variant: variant, user: user, assembler: stubbed_assembler) }
let(:user) { nil }
it { within_block_is_expected.to raise_error(Articles::Feeds::RelevancyLever::ConfigurationError) }
end
end
end
end