diff --git a/experimenter/experimenter/experiments/constants.py b/experimenter/experimenter/experiments/constants.py index b8a9248a6..a6b5b2f78 100644 --- a/experimenter/experimenter/experiments/constants.py +++ b/experimenter/experimenter/experiments/constants.py @@ -692,7 +692,8 @@ class AnalysisWindow(models.TextChoices): DAILY_ACTIVE_USERS = "client_level_daily_active_users_v2" DAYS_OF_USE = "days_of_use" - RETENTION = "retained" + RETENTION_WEEK_2 = "week_2_retention" + RETENTION_WEEK_4 = "week_4_retention" RETENTION_3_DAYS = "active_in_last_3_days" RETENTION_3_DAYS_DESKTOP = "active_in_last_3_days_legacy" SEARCH_COUNT = "search_count" @@ -710,8 +711,17 @@ class AnalysisWindow(models.TextChoices): KPI_METRICS = [ { "group": "other_metrics", - "slug": RETENTION, + "friendly_name": "Week 2 Retention", + "slug": RETENTION_WEEK_2, "display_type": "percentage", + "description": "Users who were active in Firefox during the second week after enrollment.", # noqa + }, + { + "group": "other_metrics", + "friendly_name": "Week 4 Retention", + "slug": RETENTION_WEEK_4, + "display_type": "percentage", + "description": "Users who were active in Firefox during the fourth week after enrollment.", # noqa }, { "group": "other_metrics", diff --git a/experimenter/experimenter/experiments/migrations/0339_force_weekly_retention_results_refetch.py b/experimenter/experimenter/experiments/migrations/0339_force_weekly_retention_results_refetch.py new file mode 100644 index 000000000..ece3a4a8e --- /dev/null +++ b/experimenter/experimenter/experiments/migrations/0339_force_weekly_retention_results_refetch.py @@ -0,0 +1,38 @@ +from django.db import migrations + +BATCH_SIZE = 100 + + +def stored_analysis_start_time(results_data): + metadata = (results_data.get("v3") or {}).get("metadata") + return metadata.get("analysis_start_time") if isinstance(metadata, dict) else None + + +def force_results_refetch(apps, schema_editor): + NimbusExperiment = apps.get_model("experiments", "NimbusExperiment") + + stale_experiments = [] + for experiment in NimbusExperiment.objects.exclude(results_data=None).iterator( + chunk_size=BATCH_SIZE + ): + if stored_analysis_start_time(experiment.results_data) is None: + continue + + experiment.results_data["v3"]["metadata"]["analysis_start_time"] = None + stale_experiments.append(experiment) + + if len(stale_experiments) == BATCH_SIZE: + NimbusExperiment.objects.bulk_update(stale_experiments, ["results_data"]) + stale_experiments.clear() + + NimbusExperiment.objects.bulk_update(stale_experiments, ["results_data"]) + + +class Migration(migrations.Migration): + dependencies = [ + ("experiments", "0338_nimbusemail_rollout_phase_alter_nimbusemail_type"), + ] + + operations = [ + migrations.RunPython(force_results_refetch, migrations.RunPython.noop), + ] diff --git a/experimenter/experimenter/experiments/tests/test_migrations.py b/experimenter/experimenter/experiments/tests/test_migrations.py index 9c984e929..c29b2f9bf 100644 --- a/experimenter/experimenter/experiments/tests/test_migrations.py +++ b/experimenter/experimenter/experiments/tests/test_migrations.py @@ -97,3 +97,83 @@ def test_migration(self): self.assertEqual(non_rollout.status, "Live") self.assertEqual(non_rollout.status_next, "Live") self.assertEqual(non_rollout.publish_status, "Review") + + +class TestForceWeeklyRetentionResultsRefetchMigration(MigratorTestCase): + migrate_from = ( + "experiments", + "0338_nimbusemail_rollout_phase_alter_nimbusemail_type", + ) + migrate_to = ( + "experiments", + "0339_force_weekly_retention_results_refetch", + ) + + def prepare(self): + User = self.old_state.apps.get_model("auth", "User") + NimbusExperiment = self.old_state.apps.get_model( + "experiments", "NimbusExperiment" + ) + + owner, _ = User.objects.get_or_create( + username="test@example.com", + defaults={"email": "test@example.com"}, + ) + + NimbusExperiment.objects.create( + slug="with-analysis-start-time", + name="With analysis start time", + application="firefox-desktop", + owner=owner, + results_data={ + "v3": { + "metadata": { + "analysis_start_time": "2026-07-01T00:00:00+00:00", + "outcomes": {}, + }, + "overall": {}, + } + }, + ) + NimbusExperiment.objects.create( + slug="without-metadata", + name="Without metadata", + application="firefox-desktop", + owner=owner, + results_data={"v3": {"overall": {}}}, + ) + NimbusExperiment.objects.create( + slug="null-metadata", + name="Null metadata", + application="firefox-desktop", + owner=owner, + results_data={"v3": {"metadata": None, "overall": {}}}, + ) + NimbusExperiment.objects.create( + slug="no-results", + name="No results", + application="firefox-desktop", + owner=owner, + results_data=None, + ) + + def test_migration(self): + NimbusExperiment = self.new_state.apps.get_model( + "experiments", "NimbusExperiment" + ) + + cleared = NimbusExperiment.objects.get(slug="with-analysis-start-time") + self.assertIsNone(cleared.results_data["v3"]["metadata"]["analysis_start_time"]) + self.assertEqual(cleared.results_data["v3"]["metadata"]["outcomes"], {}) + self.assertEqual(cleared.results_data["v3"]["overall"], {}) + + untouched = NimbusExperiment.objects.get(slug="without-metadata") + self.assertEqual(untouched.results_data, {"v3": {"overall": {}}}) + + null_metadata = NimbusExperiment.objects.get(slug="null-metadata") + self.assertEqual( + null_metadata.results_data, {"v3": {"metadata": None, "overall": {}}} + ) + + no_results = NimbusExperiment.objects.get(slug="no-results") + self.assertIsNone(no_results.results_data) diff --git a/experimenter/experimenter/jetstream/client.py b/experimenter/experimenter/jetstream/client.py index f27a3c821..d8c12d7a6 100644 --- a/experimenter/experimenter/jetstream/client.py +++ b/experimenter/experimenter/jetstream/client.py @@ -346,6 +346,12 @@ def get_experiment_data(experiment: NimbusExperiment): raw_segment_data = JetstreamData(segment_data) raw_data[window][AnalysisBasis.ENROLLMENTS][segment] = raw_segment_data data = raw_segment_data.model_copy(deep=True) + if data: + data.separate_weekly_retention_data( + raw_data.get(AnalysisWindow.WEEKLY, {}) + .get(AnalysisBasis.ENROLLMENTS, {}) + .get(segment) + ) ( result_metrics, primary_metrics_set, @@ -363,36 +369,6 @@ def get_experiment_data(experiment: NimbusExperiment): if data and window == AnalysisWindow.OVERALL: # Append some values onto the incoming Jetstream data data.append_population_percentages() - weekly_data = ( - raw_data.get(AnalysisWindow.WEEKLY, {}) - .get(AnalysisBasis.ENROLLMENTS, {}) - .get(segment) - ) - week_2_retention = data.get_retention_by_window( - 2, - weekly_data, - Metric.RETENTION, - ) - has_retention = any( - point.metric == Metric.RETENTION for point in weekly_data or [] - ) - - if has_retention and not week_2_retention and segment == Segment.ALL: - runtime_errors.append( - AnalysisError( - experiment=experiment.slug, - filename="experimenter/jetstream/client.py", - func_name="get_experiment_data", - log_level="WARNING", - message=( - "Week 2 retention is unavailable because this " - "experiment did not run long enough." - ), - metric=Metric.RETENTION, - timestamp=timezone.now(), - ) - ) - data.extend(week_2_retention) # Append 3-day retention from daily data data.append_retention_3_days( raw_data.get(AnalysisWindow.DAILY, {}) @@ -406,12 +382,6 @@ def get_experiment_data(experiment: NimbusExperiment): data.append_conversion_count(primary_metrics_set) elif data and window == AnalysisWindow.WEEKLY: - data.replace_retention_weeks( - raw_data.get(AnalysisWindow.WEEKLY, {}) - .get(AnalysisBasis.ENROLLMENTS, {}) - .get(segment) - ) - # Append 3-day retention from daily data data.append_retention_3_days( raw_data.get(AnalysisWindow.DAILY, {}) @@ -430,6 +400,7 @@ def get_experiment_data(experiment: NimbusExperiment): .get(AnalysisBasis.ENROLLMENTS, {}) .get(segment) ) + ResultsObjectModel = create_results_object_model(data) data = ResultsObjectModel(result_metrics, data, experiment, window) @@ -442,6 +413,12 @@ def get_experiment_data(experiment: NimbusExperiment): raw_segment_data = JetstreamData(segment_data) raw_data[window][AnalysisBasis.EXPOSURES][segment] = raw_segment_data data = raw_segment_data.model_copy(deep=True) + if data: + data.separate_weekly_retention_data( + raw_data.get(AnalysisWindow.WEEKLY, {}) + .get(AnalysisBasis.EXPOSURES, {}) + .get(segment) + ) ( result_metrics, primary_metrics_set, @@ -459,11 +436,6 @@ def get_experiment_data(experiment: NimbusExperiment): if data and window == AnalysisWindow.OVERALL: # Append some values onto Jetstream data data.append_population_percentages() - data.append_retention_data( - raw_data.get(AnalysisWindow.WEEKLY, {}) - .get(AnalysisBasis.EXPOSURES, {}) - .get(segment) - ) # Append 3-day retention from daily data data.append_retention_3_days( raw_data.get(AnalysisWindow.DAILY, {}) @@ -477,12 +449,6 @@ def get_experiment_data(experiment: NimbusExperiment): data.append_conversion_count(primary_metrics_set) elif data and window == AnalysisWindow.WEEKLY: - data.replace_retention_weeks( - raw_data.get(AnalysisWindow.WEEKLY, {}) - .get(AnalysisBasis.EXPOSURES, {}) - .get(segment) - ) - # Append 3-day retention from daily data data.append_retention_3_days( raw_data.get(AnalysisWindow.DAILY, {}) @@ -539,17 +505,14 @@ def include_error_after_analysis_start(err): errors_experiment_overall.append(err) for e in runtime_errors: - if isinstance(e, AnalysisError): - analysis_error = e - else: - analysis_error = AnalysisError( - experiment=experiment.slug, - filename="experimenter/jetstream/client.py", - func_name="load_data_from_gcs", - log_level="WARNING", - message=e, - timestamp=timezone.now(), - ) + analysis_error = AnalysisError( + experiment=experiment.slug, + filename="experimenter/jetstream/client.py", + func_name="load_data_from_gcs", + log_level="WARNING", + message=e, + timestamp=timezone.now(), + ) errors_experiment_overall.append(analysis_error.model_dump()) errors_by_metric["experiment"] = errors_experiment_overall diff --git a/experimenter/experimenter/jetstream/models.py b/experimenter/experimenter/jetstream/models.py index 4070b53e8..95cf61cb7 100644 --- a/experimenter/experimenter/jetstream/models.py +++ b/experimenter/experimenter/jetstream/models.py @@ -28,6 +28,7 @@ class BranchComparison(StrEnum): class Metric(StrEnum): RETENTION = "retained" + WEEKLY_RETENTION = "week_{}_retention" RETENTION_3_DAYS = "active_in_last_3_days" RETENTION_3_DAYS_LEGACY = "active_in_last_3_days_legacy" SEARCH = "search_count" @@ -78,7 +79,6 @@ class Group(StrEnum): Group.SEARCH: SEARCH_METRICS, Group.USAGE: USAGE_METRICS, } -RETENTION_2_WEEKS_WINDOW_INDEX = 2 RETENTION_3_DAYS_WINDOW_INDEX = 4 RETENTION_3_DAYS_METRICS = (Metric.RETENTION_3_DAYS, Metric.RETENTION_3_DAYS_LEGACY) @@ -154,29 +154,34 @@ def get_retention_by_window(self, window_index, data, metric): jetstream_data_point for jetstream_data_point in data if jetstream_data_point.window_index == str(window_index) - and jetstream_data_point.metric == metric.value + and jetstream_data_point.metric == metric ] - def append_retention_data(self, weekly_data): - # Only use two-week retention data. - retention_data = self.get_retention_by_window( - RETENTION_2_WEEKS_WINDOW_INDEX, weekly_data, Metric.RETENTION - ) + def separate_weekly_retention_data(self, weekly_data): + # Replace "retained" with one week_N_retention metric per week in "weekly_data", + # skipping week 1. Points are copied so the shared "weekly_data" is untouched. + retention_data = [] - self.extend(retention_data) + for jetstream_data_point in weekly_data or []: + if ( + jetstream_data_point.metric == Metric.RETENTION + and jetstream_data_point.window_index != "1" + ): + retention_data_point = jetstream_data_point.model_copy() + retention_data_point.metric = Metric.WEEKLY_RETENTION.format( + retention_data_point.window_index + ) + retention_data.append(retention_data_point) - def replace_retention_weeks(self, weekly_data): - # Remove all weekly retention data except for week 2. - retention_data = self.get_retention_by_window( - RETENTION_2_WEEKS_WINDOW_INDEX, weekly_data, Metric.RETENTION - ) + self.remove_retention_data() + self.extend(retention_data) + def remove_retention_data(self): self.root = [ jetstream_data_point for jetstream_data_point in self.root if jetstream_data_point.metric != Metric.RETENTION ] - self.extend(retention_data) def get_retention_3_days_by_window(self, window_index, daily_data): retention_data = [] @@ -306,11 +311,10 @@ def __init__( # Need window index for weekly DataPoint objects and for storing # significance for each window. Overall should always be 1 because - # there is only ever one overall window, except retained data which - # is pulled from week 2. + # there is only ever one overall window. window_index = ( "1" - if window == AnalysisWindow.OVERALL and metric != Metric.RETENTION + if window == AnalysisWindow.OVERALL else jetstream_data_point.window_index ) diff --git a/experimenter/experimenter/jetstream/results_manager.py b/experimenter/experimenter/jetstream/results_manager.py index bbb605f35..ad9307a49 100644 --- a/experimenter/experimenter/jetstream/results_manager.py +++ b/experimenter/experimenter/jetstream/results_manager.py @@ -401,21 +401,22 @@ def append_kpi_metric_fields( kpi["slug"], kpi["group"], analysis_basis, segment, reference_branch ) - if ( - kpi["slug"] == NimbusConstants.RETENTION_3_DAYS - or kpi["slug"] == NimbusConstants.RETENTION_3_DAYS_DESKTOP - ): - kpi["displayed_window"] = "Day 4" - # TODO: EXP-5498 - We are still unclear on the best window to show for weekly - # retention, so for now we are defaulting to showing whatever window the - # latest results data is available for. Once we have clarity, we can - # hardcode a displayed_window value like we do for 3DR. - # if kpi["slug"] == NimbusConstants.RETENTION: - # kpi["displayed_window"] = "Week 2" + match kpi["slug"]: + case NimbusConstants.RETENTION_WEEK_2: + kpi["displayed_window"] = "Week 2" + case NimbusConstants.RETENTION_WEEK_4: + kpi["displayed_window"] = "Week 4" + case ( + NimbusConstants.RETENTION_3_DAYS + | NimbusConstants.RETENTION_3_DAYS_DESKTOP + ): + kpi["displayed_window"] = "Day 4" def get_remaining_metrics_metadata( self, exclude_slugs=None, analysis_basis=None, segment=None, reference_branch=None ): + from experimenter.jetstream.models import Metric + analysis_data = ( self.experiment.results_data.get("v3", {}) if self.experiment.results_data @@ -425,11 +426,15 @@ def get_remaining_metrics_metadata( metadata = analysis_data.get("metadata", {}) metrics_metadata = metadata.get("metrics", {}) if metadata else {} defaults = [] + retention_prefix, retention_suffix = Metric.WEEKLY_RETENTION.split("{}") for group, default_metrics in other_metrics.items(): for slug, metric_friendly_name in default_metrics.items(): if exclude_slugs and slug in exclude_slugs: continue + retention_week = slug.removeprefix(retention_prefix).removesuffix( + retention_suffix + ) defaults.append( { "slug": slug, @@ -451,6 +456,10 @@ def get_remaining_metrics_metadata( "has_data": self.metric_has_data( slug, group, analysis_basis, segment, reference_branch ), + "displayed_window": (f"Week {retention_week}") + if slug.startswith(retention_prefix) + and slug.endswith(retention_suffix) + else None, } ) @@ -547,7 +556,11 @@ def get_outcome_metrics(outcome_metrics): } remaining_metrics = self.get_remaining_metrics_metadata( - exclude_slugs=all_outcome_metric_slugs, + exclude_slugs=[ + *all_outcome_metric_slugs, + NimbusConstants.RETENTION_WEEK_2, + NimbusConstants.RETENTION_WEEK_4, + ], analysis_basis=analysis_basis, segment=segment, reference_branch=reference_branch, diff --git a/experimenter/experimenter/jetstream/tests/constants.py b/experimenter/experimenter/jetstream/tests/constants.py index 47fc890b4..a54a119a0 100644 --- a/experimenter/experimenter/jetstream/tests/constants.py +++ b/experimenter/experimenter/jetstream/tests/constants.py @@ -133,7 +133,6 @@ def get_significance_data_row(cls, VARIANT_POSITIVE_SIGNIFICANCE_DATA_ROW): VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.lower = -5.0 VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.metric = Metric.RETENTION VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.statistic = Statistic.BINOMIAL - VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.window_index = "2" CONTROL_NEUTRAL_SIGNIFICANCE_DATA_ROW = ( VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.model_copy() @@ -218,9 +217,9 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_DAILY_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_C.model_copy(update={"window_index": "2"}), + DATA_POINT_C, SignificanceData( - daily={"2": Significance.NEGATIVE.value}, weekly={}, overall={} + daily={"1": Significance.NEGATIVE.value}, weekly={}, overall={} ), comparison_to_branch="control", ) @@ -235,8 +234,8 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_WEEKLY_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_C.model_copy(update={"window_index": "2"}), - SignificanceData(weekly={"2": Significance.NEGATIVE.value}, overall={}), + DATA_POINT_C, + SignificanceData(weekly={"1": Significance.NEGATIVE.value}, overall={}), comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_OVERALL_NEUTRAL_CONTROL = cls.get_difference_metric_data( @@ -250,15 +249,15 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_OVERALL_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_D.model_copy(update={"window_index": "2"}), - SignificanceData(weekly={}, overall={"2": Significance.NEGATIVE.value}), + DATA_POINT_D, + SignificanceData(weekly={}, overall={"1": Significance.NEGATIVE.value}), is_retention=True, comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_DAILY_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_B.model_copy(update={"window_index": "2"}), + DATA_POINT_B, SignificanceData( - daily={"2": Significance.NEUTRAL.value}, weekly={}, overall={} + daily={"1": Significance.NEUTRAL.value}, weekly={}, overall={} ), comparison_to_branch="variant", ) @@ -277,8 +276,8 @@ def get_differences( comparison_to_branch="variant", ) DIFFERENCE_METRIC_DATA_WEEKLY_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_B.model_copy(update={"window_index": "2"}), - SignificanceData(weekly={"2": Significance.NEUTRAL.value}, overall={}), + DATA_POINT_B, + SignificanceData(weekly={"1": Significance.NEUTRAL.value}, overall={}), comparison_to_branch="variant", ) DIFFERENCE_METRIC_DATA_WEEKLY_POSITIVE_VARIANT = cls.get_difference_metric_data( @@ -292,8 +291,8 @@ def get_differences( comparison_to_branch="variant", ) DIFFERENCE_METRIC_DATA_OVERALL_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_E.model_copy(update={"window_index": "2"}), - SignificanceData(weekly={}, overall={"2": Significance.NEUTRAL.value}), + DATA_POINT_E, + SignificanceData(weekly={}, overall={"1": Significance.NEUTRAL.value}), is_retention=True, comparison_to_branch="variant", ) @@ -658,8 +657,6 @@ def get_test_data(cls, primary_outcomes): VARIANT_DATA_DEFAULT_METRIC_ROW_DAU_IMPACT.model_dump(exclude_none=True), VARIANT_DATA_DEFAULT_METRIC_ROW_BINOMIAL.model_dump(exclude_none=True), VARIANT_POSITIVE_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), - VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), - CONTROL_NEUTRAL_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), BROKEN_STATISTIC_DATA_ROW.model_dump(exclude_none=True), VARIANT_BROKEN_STATISTIC_DATA_ROW.model_dump(exclude_none=True), ] @@ -677,10 +674,6 @@ def get_test_data(cls, primary_outcomes): EXPOSURES_VARIANT_POSITIVE_SIGNIFICANCE_DATA_ROW.model_dump( exclude_none=True ), - EXPOSURES_VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.model_dump( - exclude_none=True - ), - EXPOSURES_CONTROL_NEUTRAL_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), EXPOSURES_BROKEN_STATISTIC_DATA_ROW.model_dump(exclude_none=True), VARIANT_EXPOSURES_BROKEN_STATISTIC_DATA_ROW.model_dump(exclude_none=True), ] @@ -787,11 +780,6 @@ def get_test_data(cls, primary_outcomes): exclude_none=True ), "another_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), - "retained": ( - DIFFERENCE_METRIC_DATA_DAILY_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -821,11 +809,6 @@ def get_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_A.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_DAILY_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -848,11 +831,6 @@ def get_test_data(cls, primary_outcomes): exclude_none=True ), "another_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), - "retained": ( - DIFFERENCE_METRIC_DATA_WEEKLY_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -882,11 +860,6 @@ def get_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_A.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_WEEKLY_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -914,11 +887,6 @@ def get_test_data(cls, primary_outcomes): "default_browser_action": EMPTY_METRIC_DATA.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_OVERALL_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -950,11 +918,6 @@ def get_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_F.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_OVERALL_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), "custom_metric": EMPTY_METRIC_DATA.model_dump(exclude_none=True), }, }, @@ -1134,8 +1097,6 @@ def get_partial_exposures_test_data(cls, primary_outcomes): CONTROL_DATA_ROW.model_dump(exclude_none=True), VARIANT_DATA_ROW.model_dump(exclude_none=True), VARIANT_POSITIVE_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), - VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), - CONTROL_NEUTRAL_SIGNIFICANCE_DATA_ROW.model_dump(exclude_none=True), ] DAILY_EXPOSURES_DATA = [ EXPOSURES_CONTROL_DATA_ROW.model_dump(exclude_none=True), @@ -1222,11 +1183,6 @@ def get_partial_exposures_test_data(cls, primary_outcomes): "identity": ABSOLUTE_METRIC_DATA_A.model_dump(exclude_none=True), "some_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), "another_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), - "retained": ( - DIFFERENCE_METRIC_DATA_DAILY_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), }, }, }, @@ -1249,23 +1205,12 @@ def get_partial_exposures_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_A.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_DAILY_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), }, }, }, } FORMATTED_DAILY_EXPOSURES_DATA = deepcopy(FORMATTED_DAILY_BASE) - del FORMATTED_DAILY_EXPOSURES_DATA["control"]["branch_data"][Group.OTHER][ - "retained" - ] - del FORMATTED_DAILY_EXPOSURES_DATA["variant"]["branch_data"][Group.OTHER][ - "retained" - ] del FORMATTED_DAILY_EXPOSURES_DATA["control"]["branch_data"][Group.SEARCH][ "search_count" ] @@ -1324,11 +1269,6 @@ def get_partial_exposures_test_data(cls, primary_outcomes): "identity": ABSOLUTE_METRIC_DATA_A.model_dump(exclude_none=True), "some_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), "another_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), - "retained": ( - DIFFERENCE_METRIC_DATA_WEEKLY_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), }, }, }, @@ -1351,19 +1291,12 @@ def get_partial_exposures_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_A.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_WEEKLY_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), }, }, }, } WEEKLY_EXPOSURES_DATA = deepcopy(WEEKLY_BASE) - del WEEKLY_EXPOSURES_DATA["control"]["branch_data"][Group.OTHER]["retained"] - del WEEKLY_EXPOSURES_DATA["variant"]["branch_data"][Group.OTHER]["retained"] del WEEKLY_EXPOSURES_DATA["control"]["branch_data"][Group.SEARCH]["search_count"] del WEEKLY_EXPOSURES_DATA["variant"]["branch_data"][Group.SEARCH]["search_count"] @@ -1408,11 +1341,6 @@ def get_partial_exposures_test_data(cls, primary_outcomes): ), "some_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), "another_count": EMPTY_METRIC_DATA.model_dump(exclude_none=True), - "retained": ( - DIFFERENCE_METRIC_DATA_OVERALL_NEUTRAL_VARIANT.model_dump( - exclude_none=True - ) - ), }, }, }, @@ -1437,19 +1365,12 @@ def get_partial_exposures_test_data(cls, primary_outcomes): "another_count": ABSOLUTE_METRIC_DATA_F.model_dump( exclude_none=True ), - "retained": ( - DIFFERENCE_METRIC_DATA_OVERALL_NEGATIVE_CONTROL.model_dump( - exclude_none=True - ) - ), }, }, }, } OVERALL_EXPOSURES_DATA = deepcopy(OVERALL_BASE) - del OVERALL_EXPOSURES_DATA["control"]["branch_data"][Group.OTHER]["retained"] - del OVERALL_EXPOSURES_DATA["variant"]["branch_data"][Group.OTHER]["retained"] del OVERALL_EXPOSURES_DATA["control"]["branch_data"][Group.SEARCH]["search_count"] del OVERALL_EXPOSURES_DATA["variant"]["branch_data"][Group.SEARCH]["search_count"] @@ -1538,7 +1459,6 @@ def get_significance_data_row(cls, VARIANT_POSITIVE_SIGNIFICANCE_DATA_ROW): VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.lower = 0.0 VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.metric = Metric.RETENTION VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.statistic = Statistic.BINOMIAL - VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.window_index = "2" CONTROL_NEUTRAL_SIGNIFICANCE_DATA_ROW = ( VARIANT_NEGATIVE_SIGNIFICANCE_DATA_ROW.model_copy() @@ -1608,7 +1528,7 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_DAILY_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_C.model_copy(update={"window_index": "2"}), + DATA_POINT_C, SignificanceData(daily={}, weekly={}, overall={}), comparison_to_branch="control", ) @@ -1623,7 +1543,7 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_WEEKLY_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_C.model_copy(update={"window_index": "2"}), + DATA_POINT_C, SignificanceData(weekly={}, overall={}), comparison_to_branch="control", ) @@ -1638,13 +1558,13 @@ def get_differences( comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_OVERALL_NEGATIVE_CONTROL = cls.get_difference_metric_data( - DATA_POINT_D.model_copy(update={"window_index": "2"}), + DATA_POINT_D, SignificanceData(weekly={}, overall={}), is_retention=True, comparison_to_branch="control", ) DIFFERENCE_METRIC_DATA_DAILY_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_B.model_copy(update={"window_index": "2"}), + DATA_POINT_B, SignificanceData(daily={}, weekly={}, overall={}), comparison_to_branch="variant", ) @@ -1659,7 +1579,7 @@ def get_differences( comparison_to_branch="variant", ) DIFFERENCE_METRIC_DATA_WEEKLY_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_B.model_copy(update={"window_index": "2"}), + DATA_POINT_B, SignificanceData(weekly={}, overall={}), comparison_to_branch="variant", ) @@ -1674,7 +1594,7 @@ def get_differences( comparison_to_branch="variant", ) DIFFERENCE_METRIC_DATA_OVERALL_NEUTRAL_VARIANT = cls.get_difference_metric_data( - DATA_POINT_E.model_copy(update={"window_index": "2"}), + DATA_POINT_E, SignificanceData(weekly={}, overall={}), is_retention=True, comparison_to_branch="variant", diff --git a/experimenter/experimenter/jetstream/tests/test_jetstream_data.py b/experimenter/experimenter/jetstream/tests/test_jetstream_data.py index 6ac214bba..3f960f74b 100644 --- a/experimenter/experimenter/jetstream/tests/test_jetstream_data.py +++ b/experimenter/experimenter/jetstream/tests/test_jetstream_data.py @@ -76,74 +76,68 @@ def test_append_retention_3_days_extracts_legacy_data(self): self.assertIn(retention, data) - def test_append_retention_data_ignores_week_1_only_data(self): - week_1_retention = JetstreamDataPoint( + def test_remove_retention_data(self): + retained = JetstreamDataPoint( metric=Metric.RETENTION, statistic=Statistic.BINOMIAL, branch="control", point=0.65, segment=Segment.ALL, - window_index="1", ) + identity = JetstreamTestData.get_identity_row() + data = JetstreamData([retained, identity]) - data = JetstreamData([]) - data.append_retention_data([week_1_retention]) + data.remove_retention_data() - self.assertNotIn(week_1_retention, data) + self.assertEqual(data.root, [identity]) - def test_append_retention_data_uses_week_2_data(self): - week_2_retention = JetstreamDataPoint( - metric=Metric.RETENTION, - statistic=Statistic.BINOMIAL, - branch="control", - point=0.65, - segment=Segment.ALL, - window_index="2", + def test_separate_weekly_retention_data_splits_retention_by_week(self): + weekly_data = JetstreamData( + [ + JetstreamDataPoint( + metric=Metric.RETENTION, + statistic=Statistic.BINOMIAL, + branch="control", + point=week / 10, + segment=Segment.ALL, + window_index=str(week), + ) + for week in (1, 2, 3, 4, 5, 6) + ] ) + transformed_data = JetstreamData([]) - data = JetstreamData([]) - data.append_retention_data([week_2_retention]) + transformed_data.separate_weekly_retention_data(weekly_data) - self.assertIn(week_2_retention, data) + self.assertEqual( + [ + (point.metric, point.window_index, point.point) + for point in transformed_data + ], + [ + (Metric.WEEKLY_RETENTION.format(2), "2", 0.2), + (Metric.WEEKLY_RETENTION.format(3), "3", 0.3), + (Metric.WEEKLY_RETENTION.format(4), "4", 0.4), + (Metric.WEEKLY_RETENTION.format(5), "5", 0.5), + (Metric.WEEKLY_RETENTION.format(6), "6", 0.6), + ], + ) - def test_replace_retention_replaces_existing_entries(self): - existing_retention_1 = JetstreamDataPoint( + def test_separate_weekly_retention_data_ignores_week_1_only_data(self): + week_1_retention = JetstreamDataPoint( metric=Metric.RETENTION, statistic=Statistic.BINOMIAL, branch="control", - point=0.5, + point=0.65, segment=Segment.ALL, window_index="1", ) - kept_retention = JetstreamDataPoint( - metric=Metric.RETENTION, - statistic=Statistic.BINOMIAL, - branch="control", - point=0.25, - segment=Segment.ALL, - window_index="2", - ) - existing_retention_2 = JetstreamDataPoint( - metric=Metric.RETENTION, - statistic=Statistic.BINOMIAL, - branch="control", - point=0.25, - segment=Segment.ALL, - window_index="3", - ) + identity = JetstreamTestData.get_identity_row() + data = JetstreamData([identity]) - data = JetstreamData( - [ - existing_retention_1, - kept_retention, - existing_retention_2, - ] - ) - data.replace_retention_weeks(data) + data.separate_weekly_retention_data(JetstreamData([week_1_retention])) - self.assertNotIn(existing_retention_1, data) - self.assertNotIn(existing_retention_2, data) - self.assertIn(kept_retention, data) + self.assertEqual(data.root, [identity]) def test_replace_retention_3_days_replaces_existing_entries(self): existing_retention_1 = JetstreamDataPoint( diff --git a/experimenter/experimenter/jetstream/tests/test_results_manager.py b/experimenter/experimenter/jetstream/tests/test_results_manager.py index c8e266a89..ab74e6864 100644 --- a/experimenter/experimenter/jetstream/tests/test_results_manager.py +++ b/experimenter/experimenter/jetstream/tests/test_results_manager.py @@ -933,8 +933,8 @@ def test_metric_areas_created_correctly(self): "all": { "branch-a": { "branch_data": { - "other_metrics": { - "retained": { + "search_metrics": { + "search_count": { "absolute": { "all": [ { @@ -978,8 +978,8 @@ def test_metric_areas_created_correctly(self): }, "branch-b": { "branch_data": { - "other_metrics": { - "retained": { + "search_metrics": { + "search_count": { "absolute": { "all": [ { @@ -1035,7 +1035,7 @@ def test_metric_areas_created_correctly(self): self.assertIn("Notable Changes", metric_areas) self.assertIn( - "retained", + "search_count", [metric["slug"] for metric in metric_areas["Notable Changes"]["metrics"]], ) @@ -1299,9 +1299,26 @@ def test_get_branch_data_returns_correct_data(self): [ { "group": "other_metrics", - "friendly_name": "Retention", - "slug": "retained", - "description": "Retention description", + "friendly_name": "Week 2 Retention", + "slug": "week_2_retention", + "displayed_window": "Week 2", + "description": ( + "Users who were active in Firefox during the second week " + "after enrollment." + ), + "display_type": "percentage", + "overall_change": MetricSignificance.NEUTRAL, + "has_data": False, + }, + { + "group": "other_metrics", + "friendly_name": "Week 4 Retention", + "slug": "week_4_retention", + "displayed_window": "Week 4", + "description": ( + "Users who were active in Firefox during the fourth week " + "after enrollment." + ), "display_type": "percentage", "overall_change": MetricSignificance.NEUTRAL, "has_data": False, @@ -1339,9 +1356,26 @@ def test_get_branch_data_returns_correct_data(self): [ { "group": "other_metrics", - "friendly_name": "Retention", - "slug": "retained", - "description": "Retention description", + "friendly_name": "Week 2 Retention", + "displayed_window": "Week 2", + "slug": "week_2_retention", + "description": ( + "Users who were active in Firefox during the second week " + "after enrollment." + ), + "display_type": "percentage", + "overall_change": MetricSignificance.NEUTRAL, + "has_data": False, + }, + { + "group": "other_metrics", + "friendly_name": "Week 4 Retention", + "displayed_window": "Week 4", + "slug": "week_4_retention", + "description": ( + "Users who were active in Firefox during the fourth week " + "after enrollment." + ), "display_type": "percentage", "overall_change": MetricSignificance.NEUTRAL, "has_data": False, @@ -1396,9 +1430,19 @@ def test_get_kpi_metrics_returns_correct_metrics( "friendly_name": "Search Count", "description": "Search Count description", }, - "retained": { - "friendly_name": "Retention", - "description": "Retention description", + "week_2_retention": { + "friendly_name": "Week 2 Retention", + "description": ( + "Users who were active in Firefox during the second " + "week after enrollment." + ), + }, + "week_4_retention": { + "friendly_name": "Week 4 Retention", + "description": ( + "Users who were active in Firefox during the fourth " + "week after enrollment." + ), }, "active_in_last_3_days_legacy": { "friendly_name": "3-Day Retention", @@ -1549,7 +1593,8 @@ def test_get_default_metrics_with_exclusions(self): }, "other_metrics": { "other_metrics": { - "retained": "2 Week Retention", + "retained": "Week 2 Retention", + "week_6_retention": "Week 6 Retention", "search_count": "Search Count", } }, @@ -1564,6 +1609,10 @@ def test_get_default_metrics_with_exclusions(self): self.assertIn("retained", metric_slugs) self.assertNotIn("search_count", metric_slugs) + weekly_retention = next( + metric for metric in remaining_metrics if metric["slug"] == "week_6_retention" + ) + self.assertEqual(weekly_retention["displayed_window"], "Week 6") @parameterized.expand( [ @@ -2888,7 +2937,8 @@ def test_metrics_have_data_different_windows(self, results_data, expected): NimbusExperiment.Application.DESKTOP, [ "client_level_daily_active_users_v2", - "retained", + "week_2_retention", + "week_4_retention", "search_count", "active_in_last_3_days_legacy", ], @@ -2897,7 +2947,8 @@ def test_metrics_have_data_different_windows(self, results_data, expected): NimbusExperiment.Application.FENIX, [ "client_level_daily_active_users_v2", - "retained", + "week_2_retention", + "week_4_retention", "search_count", "active_in_last_3_days", ], @@ -2906,7 +2957,8 @@ def test_metrics_have_data_different_windows(self, results_data, expected): NimbusExperiment.Application.IOS, [ "client_level_daily_active_users_v2", - "retained", + "week_2_retention", + "week_4_retention", "search_count", "active_in_last_3_days", ], diff --git a/experimenter/experimenter/jetstream/tests/test_tasks.py b/experimenter/experimenter/jetstream/tests/test_tasks.py index 141bc9b1e..b7d03ee6d 100644 --- a/experimenter/experimenter/jetstream/tests/test_tasks.py +++ b/experimenter/experimenter/jetstream/tests/test_tasks.py @@ -1068,7 +1068,7 @@ def mock_jetstream_data_by_window(_, window): mock_get_errors.return_value = None tasks.fetch_experiment_data(experiment.id) - def test_results_data_warns_when_week_2_retention_is_missing(self): + def test_results_data_includes_weekly_retention_in_overall_window(self): experiment = NimbusExperimentFactory.create_with_lifecycle( NimbusExperimentFactory.Lifecycles.CREATED, ) @@ -1082,11 +1082,12 @@ def mock_jetstream_data_by_window(_, window): "metric": Metric.RETENTION, "statistic": "binomial", "branch": "control", - "point": 0.5, + "point": week / 10, "segment": "all", "analysis_basis": "enrollments", - "window_index": "1", + "window_index": str(week), } + for week in (1, 2, 4) ] if window == AnalysisWindow.OVERALL: return [ @@ -1110,18 +1111,17 @@ def mock_jetstream_data_by_window(_, window): mock_get_data.side_effect = mock_jetstream_data_by_window mock_get_metadata.return_value = None mock_get_errors.return_value = None - tasks.fetch_experiment_data(experiment.id) experiment.refresh_from_db() - errors = experiment.results_data["v3"]["errors"]["experiment"] - self.assertEqual(len(errors), 1) - self.assertEqual(errors[0]["metric"], Metric.RETENTION) - self.assertEqual( - errors[0]["message"], - "Week 2 retention is unavailable because this experiment did not run " - "long enough.", - ) + overall_metrics = experiment.results_data["v3"]["overall"]["enrollments"]["all"][ + "control" + ]["branch_data"][Group.OTHER] + + self.assertIn(Metric.WEEKLY_RETENTION.format(2), overall_metrics) + self.assertIn(Metric.WEEKLY_RETENTION.format(4), overall_metrics) + self.assertNotIn(Metric.WEEKLY_RETENTION.format(1), overall_metrics) + self.assertNotIn(Metric.RETENTION, overall_metrics) @parameterized.expand( [ diff --git a/experimenter/experimenter/nimbus_ui/templates/common/metric_popout.html b/experimenter/experimenter/nimbus_ui/templates/common/metric_popout.html index d160fb7ad..a4735beae 100644 --- a/experimenter/experimenter/nimbus_ui/templates/common/metric_popout.html +++ b/experimenter/experimenter/nimbus_ui/templates/common/metric_popout.html @@ -106,7 +106,7 @@

Overview

{% with weekly_metric_data=all_weekly_metric_data|dict_get:metric_info.slug %} - {% if weekly_metric_data.has_weekly_data and not metric_info.slug in NimbusUIConstants.HIDDEN_WEEKLY_METRICS %} + {% if weekly_metric_data.has_weekly_data and not metric_info.slug in hidden_weekly_metrics %}

Weekly breakdown

@@ -168,7 +168,7 @@

Weekly breakdown

{% endif %} {% endwith %} {% with daily_metric_data=all_daily_metric_data|dict_get:metric_info.slug %} - {% if daily_metric_data.has_daily_data and not metric_info.slug in NimbusUIConstants.HIDDEN_DAILY_METRICS %} + {% if daily_metric_data.has_daily_data and not metric_info.slug in hidden_daily_metrics %}

Daily breakdown

diff --git a/experimenter/experimenter/nimbus_ui/tests/test_views.py b/experimenter/experimenter/nimbus_ui/tests/test_views.py index b016745ba..9d4ac8535 100644 --- a/experimenter/experimenter/nimbus_ui/tests/test_views.py +++ b/experimenter/experimenter/nimbus_ui/tests/test_views.py @@ -36,6 +36,7 @@ NimbusVersionedSchemaFactory, TagFactory, ) +from experimenter.jetstream.models import Metric from experimenter.kinto.tasks import ( nimbus_check_kinto_push_queue_by_collection, nimbus_synchronize_preview_experiments_in_kinto, @@ -3684,6 +3685,42 @@ def test_render_to_response(self): self.assertEqual(response.context["experiment"], experiment) self.assertTemplateUsed(response, "nimbus_experiments/results.html") + @patch("experimenter.nimbus_ui.views.ExperimentResultsManager.get_metric_data") + def test_results_view_hides_weekly_retention_metric_breakdowns( + self, mock_get_metric_data + ): + retention_slug = Metric.WEEKLY_RETENTION.format(6) + mock_get_metric_data.return_value = { + "Other Metrics": { + "metrics": [ + { + "slug": retention_slug, + "group": "other_metrics", + "has_data": False, + }, + { + "slug": "days_of_use", + "group": "other_metrics", + "has_data": False, + }, + ], + "data": {}, + "label_details": None, + } + } + experiment = NimbusExperimentFactory.create_with_lifecycle( + NimbusExperimentFactory.Lifecycles.ENDING_APPROVE_APPROVE, + ) + + response = self.client.get( + reverse("nimbus-ui-results", kwargs={"slug": experiment.slug}), + ) + + self.assertIn(retention_slug, response.context["hidden_weekly_metrics"]) + self.assertIn(retention_slug, response.context["hidden_daily_metrics"]) + self.assertNotIn("days_of_use", response.context["hidden_weekly_metrics"]) + self.assertNotIn("days_of_use", response.context["hidden_daily_metrics"]) + @parameterized.expand( [ ( diff --git a/experimenter/experimenter/nimbus_ui/views.py b/experimenter/experimenter/nimbus_ui/views.py index 9c5c8edaf..f8117857b 100644 --- a/experimenter/experimenter/nimbus_ui/views.py +++ b/experimenter/experimenter/nimbus_ui/views.py @@ -21,6 +21,7 @@ NimbusVersionedSchema, Tag, ) +from experimenter.jetstream.models import Metric from experimenter.jetstream.results_manager import ExperimentResultsManager from experimenter.nimbus_ui.constants import ( METRICS_MIN_BOUNDS_WIDTH, @@ -842,6 +843,26 @@ def get_context_data(self, **kwargs): ) context["metric_area_data"] = all_metrics + metric_slugs = { + metric["slug"] + for area_data in all_metrics.values() + for metric in area_data.get("metrics", []) + } + retention_prefix, retention_suffix = Metric.WEEKLY_RETENTION.split("{}") + weekly_retention_slugs = { + slug + for slug in metric_slugs + if slug.startswith(retention_prefix) and slug.endswith(retention_suffix) + } + context["hidden_weekly_metrics"] = { + *NimbusUIConstants.HIDDEN_WEEKLY_METRICS, + *weekly_retention_slugs, + } + context["hidden_daily_metrics"] = { + *NimbusUIConstants.HIDDEN_DAILY_METRICS, + *weekly_retention_slugs, + } + context["ask_experimenter_slack_link"] = settings.ASK_EXPERIMENTER_SLACK_LINK relative_metric_changes = {}