Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Significance to Shared Package #2379

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Refactor Significance to Shared Package #2379

merged 2 commits into from
Apr 12, 2024

Conversation

lukesonnet
Copy link
Collaborator

@lukesonnet lukesonnet commented Apr 11, 2024

In prep for more robust alerting and notifications, this PR refactors the metric significance logic to the shared package where it will be used by back-end code for building metric notifications.

TODO:

  • Share code on how to use this to build notifs
  • Check for other instances of significance logic in the code base to centralize business logic (there is the sendSignificanceEmail but that is well out of date and should be deprecated eventually)

How this can be used to build notifs (pseudo-code that belongs in shared/experiments.ts for discussion):

function getMetricStatusesInSnapshot(
  metrics: ExperimentMetricInterface[],
  baseline: SnapshotVariation,
  variation: SnapshotVariation,
  // some org settings
  statsEngine: StatsEngine,
  metricDefaults: MetricDefaults,
  confidenceLevel: number,
  pValueThreshold: number,
): Record<string,  "won" | "lost" | "draw" | ""> {


  const ciUpper = confidenceLevel || 0.95;
  const ciLower = 1 - ciUpper;
  // may be a better way to handle metrics for which we cannot compute results for some reason
  // than just omitting them from response like I do here
  const metricStatuses: Record<string,  "won" | "lost" | "draw" | ""> = {};

  metrics.forEach((metric) => {
    const baselineStats = baseline.metrics[metric.id];
    const variationStats = variation.metrics[metric.id];
    if (!baselineStats || !variationStats) return;

    const {resultsStatus} = getMetricResultStatus({metric, metricDefaults, baseline: baselineStats, stats: variationStats, ciLower, ciUpper, pValueThreshold, statsEngine: statsEngine});
    
    metricStatuses[metric.id] = resultsStatus
  });
  return metricStatuses;
}


// FIRST approach uses the full last snapshot object
function getExperimentMetricNotifications(
  currentSnapshot: ExperimentSnapshotInterface,
  lastSnapshot: ExperimentSnapshotInterface | undefined,
  // some org settings
  metricDefaults: MetricDefaults,
  confidenceLevel: number,
  pValueThreshold: number,
  // the mettings they've selected to notify on
  metricsToNotify: ExperimentMetricInterface[], // await getMetricsByIds(context, metricIdsToNotify);
): Record<string,  "won" | "lost" | "">[] {

  const notifications: Record<string,  "won" | "lost" | "">[] = [];

  // Only fire events for overall dimension values, this logic should probably not be specific
  // to this method 
  if (currentSnapshot.dimension) return notifications;
  // TODO only automatic updates?
  // TODO validate both snapshots have same N variations
  

  const analysis = currentSnapshot.analyses?.[0];
  const results = analysis.results?.[0]
  if (!results) return notifications;
  const baselineVariation = results.variations[0];

  const lastAnalysis = lastSnapshot?.analyses?.[0];
  const lastResults = lastAnalysis?.results?.[0];
  const lastBaselineVariation = lastResults?.variations[0];

  results.variations.forEach((variation, i) => {
    if (i === 0) return;
    const variationNotifications: Record<string,  "won" | "lost" | ""> = {};
    const currentMetricStatuses = getMetricStatusesInSnapshot(metricsToNotify, baselineVariation, variation, analysis.settings.statsEngine, metricDefaults, confidenceLevel, pValueThreshold);

    const lastVariation = lastResults?.variations[i];
    const lastMetricStatuses = lastAnalysis && lastBaselineVariation && lastVariation &&  getMetricStatusesInSnapshot(metricsToNotify, lastBaselineVariation, lastVariation, lastAnalysis.settings.statsEngine, metricDefaults, confidenceLevel, pValueThreshold);
      
    for (const metric in currentMetricStatuses) {
      if (currentMetricStatuses[metric] === "won" && (!lastMetricStatuses || lastMetricStatuses[metric] !== "won")) {
        variationNotifications[metric] = "won"; // Metric X is now significantly winning
      }
      if (currentMetricStatuses[metric] === "" && (!!lastMetricStatuses && lastMetricStatuses[metric] !== "")) {
        variationNotifications[metric] = ""; // Metric X is no longer statistically significant
      }
      if (currentMetricStatuses[metric] === "lost" && (!lastMetricStatuses || lastMetricStatuses[metric] !== "lost")) {
        variationNotifications[metric] = "lost"; // Metric X is now significantly losing
      }
    }
    notifications.push(variationNotifications);
  });

  return notifications;

}

// SECOND approach only uses the last notification object, not the full snapshot
function getExperimentMetricNotifications2(
  currentSnapshot: ExperimentSnapshotInterface,
  lastNotifications: Record<string,  "won" | "lost" | "">[],
  // some org settings
  metricDefaults: MetricDefaults,
  confidenceLevel: number,
  pValueThreshold: number,
  // the mettings they've selected to notify on
  metricsToNotify: ExperimentMetricInterface[], // await getMetricsByIds(context, metricIdsToNotify);
): Record<string,  "won" | "lost" | "">[] {

  const notifications: Record<string,  "won" | "lost" | "">[] = [];

  // Only fire events for overall dimension values, this logic should probably not be specific
  // to this method 
  if (currentSnapshot.dimension) return notifications;
  // TODO only automatic updates?
  // TODO validate both snapshots have same N variations
  

  const analysis = currentSnapshot.analyses?.[0];
  const results = analysis.results?.[0]
  if (!results) return notifications;
  const baselineVariation = results.variations[0];

  results.variations.forEach((variation, i) => {
    if (i === 0) return;
    const variationNotifications: Record<string,  "won" | "lost" | ""> = {};
    const currentMetricStatuses = getMetricStatusesInSnapshot(metricsToNotify, baselineVariation, variation, analysis.settings.statsEngine, metricDefaults, confidenceLevel, pValueThreshold);
  
    const lastVariationNotifications = lastNotifications?.[i-1];

    for (const metric in currentMetricStatuses) {

      if (currentMetricStatuses[metric] === "won" && (!lastVariationNotifications || lastVariationNotifications[metric] !== "won")) {
        variationNotifications[metric] = "won"; // Metric X is now significantly winning
      }
      if (currentMetricStatuses[metric] === "" && (!!lastVariationNotifications && lastVariationNotifications[metric] !== "")) {
        variationNotifications[metric] = ""; // Metric X is no longer statistically significant
      }
      if (currentMetricStatuses[metric] === "lost" && (!lastVariationNotifications || lastVariationNotifications[metric] !== "lost")) {
        variationNotifications[metric] = "lost"; // Metric X is now significantly losing
      }
    }
    notifications.push(variationNotifications);
  });

  return notifications;

}

Copy link

Your preview environment pr-2379-bttf has been deployed.

Preview environment endpoints are available at:

Copy link
Member

@bryce-fitzsimons bryce-fitzsimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, low risk

@lukesonnet lukesonnet merged commit 7a75341 into main Apr 12, 2024
3 checks passed
@lukesonnet lukesonnet deleted the ls/significance branch April 12, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants