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

Fix unnecessary activation fact metric data retrieval (and two minor FE bugs) #1922

Merged
merged 3 commits into from Dec 6, 2023

Conversation

lukesonnet
Copy link
Contributor

Features and Changes

Fact metrics currently have hidden logic returning conversion windows in a few places where they shouldn't if a conversion window has not been selected.

Back end fix (SqlIntegration.ts):

FE fixes:

  • Update the mouseover in the results table to not show a window for a metric when one doesn't exist
  • We should update the old view in the config tab showing the overrides to have "none" when there is no conversion window!

Testing

Some in-app testing with fact metrics that do and do not have conversion windows, showing the correct behavior and generated SQL.

Screenshots

Metric name mouse over before, even though this metric has conversion window == off
Screenshot 2023-12-05 at 4 40 26 PM

Metric name mouse over after:
Screenshot 2023-12-05 at 4 40 03 PM

old page config tab before, even though number of purchases has no conversion window:
Screenshot 2023-12-05 at 4 50 48 PM

old page config tab after:
Screenshot 2023-12-05 at 4 51 07 PM

Back-end SQL before:

__experimentExposures AS (
    -- Viewed Experiment
    SELECT
      e.user_id as user_id,
      cast(e.variation_id as varchar) as variation,
      e.timestamp as timestamp
    FROM
      __rawExperiment e
    WHERE
      e.experiment_id = 'checkout-layout'
      AND e.timestamp >= '2021-11-22 20:23:00'
      AND e.timestamp <= '2023-10-17 17:04:00'
  ),
  __activationMetric as ( -- Metric (any order)
    SELECT
      user_id as user_id,
      1 as value,
      m.timestamp as timestamp
    FROM
      (
        SELECT
          userid as user_id,
          timestamp as timestamp,
          qty,
          amount
        FROM
          orders
      ) m
    WHERE
      m.timestamp >= '2021-11-22 20:23:00'
      AND m.timestamp <= '2023-10-20 17:04:00'
  ),
  __experimentUnits AS (
    -- One row per user
    SELECT
      e.user_id AS user_id,
      (
        CASE
          WHEN count(distinct e.variation) > 1 THEN '__multiple__'
          ELSE max(e.variation)
        END
      ) AS variation,
      MIN(e.timestamp) AS first_exposure_timestamp,
      MIN(
        (
          CASE
            WHEN a.timestamp >= e.timestamp THEN a.timestamp
            ELSE NULL
          END
        )
      ) AS first_activation_timestamp
    FROM
      __experimentExposures e
      LEFT JOIN __activationMetric a ON (a.user_id = e.user_id)
    GROUP BY
      e.user_id
  ),
  __distinctUsers AS (
    SELECT
      user_id,
      cast('All' as varchar) AS dimension,
      variation,
      first_activation_timestamp AS timestamp,
      date_trunc('day', first_exposure_timestamp) AS first_exposure_date,
      first_exposure_timestamp AS preexposure_end,
      first_exposure_timestamp - INTERVAL '336 hours' AS preexposure_start
    FROM
      __experimentUnits
    WHERE
      first_activation_timestamp IS NOT NULL
      AND first_activation_timestamp <= '2023-10-17 17:04:00'
  ),

Notice how we filter out first_activation_timestamp <= '2023-10-17 17:04:00' but we're nonsensically getting data in the activation metric CTE with m.timestamp <= '2023-10-20 17:04:00'

Now after this change this same SQL snippet is:

__experimentExposures AS (
  -- Viewed Experiment
  SELECT
    e.user_id as user_id,
    cast(e.variation_id as varchar) as variation,
    e.timestamp as timestamp
  FROM
    __rawExperiment e
  WHERE
    e.experiment_id = 'checkout-layout'
    AND e.timestamp >= '2021-11-22 20:23:00'
    AND e.timestamp <= '2023-10-17 17:04:00'
),
__activationMetric as ( -- Metric (any order)
  SELECT
    user_id as user_id,
    1 as value,
    m.timestamp as timestamp
  FROM
    (
      SELECT
        userid as user_id,
        timestamp as timestamp,
        qty,
        amount
      FROM
        orders
    ) m
  WHERE
    m.timestamp >= '2021-11-22 20:23:00'
    AND m.timestamp <= '2023-10-17 17:04:00'
),
__experimentUnits AS (
  -- One row per user
  SELECT
    e.user_id AS user_id,
    (
      CASE
        WHEN count(distinct e.variation) > 1 THEN '__multiple__'
        ELSE max(e.variation)
      END
    ) AS variation,
    MIN(e.timestamp) AS first_exposure_timestamp,
    MIN(
      (
        CASE
          WHEN a.timestamp >= e.timestamp THEN a.timestamp
          ELSE NULL
        END
      )
    ) AS first_activation_timestamp
  FROM
    __experimentExposures e
    LEFT JOIN __activationMetric a ON (a.user_id = e.user_id)
  GROUP BY
    e.user_id
),
__distinctUsers AS (
  SELECT
    user_id,
    cast('All' as varchar) AS dimension,
    variation,
    first_activation_timestamp AS timestamp,
    date_trunc('day', first_exposure_timestamp) AS first_exposure_date,
    first_exposure_timestamp AS preexposure_end,
    first_exposure_timestamp - INTERVAL '336 hours' AS preexposure_start
  FROM
    __experimentUnits
  WHERE
    first_activation_timestamp IS NOT NULL
    AND first_activation_timestamp <= '2023-10-17 17:04:00'
),

@lukesonnet lukesonnet requested a review from jdorn December 6, 2023 01:08
@lukesonnet lukesonnet changed the title Update conversion window logic for fact metrics Fix unnecessary activation fact metric data retrieval (and two minor FE bugs) Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

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

Preview environment endpoints are available at:

@lukesonnet lukesonnet merged commit a125e98 into main Dec 6, 2023
3 checks passed
@lukesonnet lukesonnet deleted the ls/fact-conversion branch December 6, 2023 16:36
bryce-fitzsimons pushed a commit that referenced this pull request Dec 12, 2023
…FE bugs) (#1922)

* Update conversion window logic for fact metrics

* CLean up language

* More proximate fix
bryce-fitzsimons added a commit that referenced this pull request Jan 12, 2024
* initial, updating existing types and methods with new bucket stuff

* moar types

* simplify targeting ui, remove stickyBucketing var

* add minBucketVersion, add reseed option for new phases

* org setting for sticky bucketing, premium feature, organize settings page

* lock SB switch based if !hasCommercialFeature

* working on rollout recommendation logic

* variation blocking ui, wip

* tweak logic kinda

* tweak

* re-add disableStickyBucketing

* ui wip

* ui wip

* ui wip

* ui wip

* ui wip

* ui wip

* ui wip

* ui wip

* Support ability to set role with SCIM request. (#1779)

* Support ability to set role with SCIM request.

* Modify cascading window logic for excluding in-progress conversions (#1921)

* Modify cascading window logic for excluding in-progress conversions

* Move fn

* Test

* Fix unnecessary activation fact metric data retrieval (and two minor FE bugs) (#1922)

* Update conversion window logic for fact metrics

* CLean up language

* More proximate fix

* Pre-compute safe dimension slices (#1873)

* Add reliable dimensions query

* Modal mostly working

* Get save working

* lint and such

* merge origin main

* Wire auto dimensions through to units and traffic query

* remove dimensions for traffic

* Fix SQL

* Bryce comments

* Move to kebab & add context

* rename && remove ff

* remove FF

* Table version

* ui/code tweaks

* Add running language

* remove console

* Remove metadata field

* Additional renaming

* Hide entry with no dimensions

* Move storing specified values directly to exposure query

* TS

* Small updates

* Fix type issue

* Rename && add lookback selector

* Cleanup

* push pixels

* Add tracking

* Blank out button for automatic dimensions

* Ensure cast to string before string comparison

---------

Co-authored-by: Bryce <bryce1@gmail.com>

* Add new rollback workflow (#1867)

* Fix bug - metric capping cannot be updated on old metrics (#1930)

* Add Random On-Screen Celebration Service (#1826)

* Add on-screen confetti celebration service to app. Launch experiment consumes it and will fire 20% of the time.

* Allow orgs to define role when creating and updating groups via SCIM (#1909)

* Adds support for setting team/group global role when creating a team or updating a team via SCIM endpoints

* SDK compatibility versioning (#1893)

* initial: new sdkVersion field, sdkid/form UI

* sdk-versioning package, getCurrentVersion method

* getCapabilities

* payload scrubbing. mostly feature complete

* BE resolveJsonModule

* update scrubbing logic to use pick

* refactor json

* isOutdated check

* add all sdks, fill in versions/capabilities

* tweaks

* get minimal safe capabilities when multi-language, add tests

* basic gating in SDK connection form

* more retrofitting

* more retrofitting

* address feedback

* address feedback

* lint

* use today's capabilities as a snapshot for default sdk version

* small fixes

* clean up scrubbing, add tests

* fix sdkconnection post endpoint

* fix model option version

* api types

* fix test

* multi-stage scrubbing, update legacy connection logic, tests

* update php for looseUnmarshalling

* address feedback

* log payload deltas instead of scrubbing until confident

* use logger

* log capabilities

* use isEqual for better payload scrub comparison (#1935)

* ui wip

* resize release plan summary. probably redo this later...

* fix hook deps

* ui wip

* block save when no changes, cleanup old code

* more clarity around SB SDK support

* refactor models based on discussions (no blockedVariations, new bool)

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* wip multi-stage modal

* confirm check

* add warnings while making changes

* cleanup

* cleanup

* fine tune ui

* remove blocked from variation (add later)

* clean up modal ui

* payload scrubbing

* remove reseed from phases modals, fix any type

* attempt saved group change detection

* lint

* move warning to release plan page, don't warn when using recommended, ui tweaks

* change targeting language, ui tweaks

* fix saved group comparison, fix release plan selector, adjust ui

* wip SB onboarding / toggle

* sticky bucketing onboarding and toggle gating + warnings

* phases: looking for targeting? -> make changes

* refactor

* retool warnings

* address some feedback

* adjust settings SB toggle

* fix saved group restrictive ANY calc

* fix saved group restrictive ANY calc again

* make changes to choices

* other fix

* tweaks

* calculate risk level per option

* phase and warning stuff

* ui tweak, edit phases show/hide button logic

* adjust warning language

* move same-phase-everyone into default release plan options

* add SB keys to payload scrubber

* implement some suggestions

* fix legacy webhook capabilities

* fix help text

* fix help text again

* update types

* ui tweaks

* remove onboarding CTAs when sdk issues detected, point to docs

* gate fallback attributes by org setting

* UI tweaks for sticky bucket onboarding and Make Changes modal

* minor ui tweaks

* Sticky bucketing docs (#2017)

* talk about new stuff in experiment-configuration, fix proxy docs (unrelated)

* sb doc stub

* sb doc stub

* sb doc wip

* change slug

* sb doc wip

* sb doc wip

* sb doc wip

* sdk docs

* Language

* de-emphasize fallbackAttribute in examples

* Update sticky bucket docs page with fallback attribute example and helper image for SDK connections

---------

Co-authored-by: Luke Sonnet <luke.sonnet@gmail.com>
Co-authored-by: Jeremy Dorn <jeremy@jeremydorn.com>

---------

Co-authored-by: Michael Knowlton <michael@growthbook.io>
Co-authored-by: Luke Sonnet <lukesonnet@users.noreply.github.com>
Co-authored-by: Adnan Chowdhury <2374625+bttf@users.noreply.github.com>
Co-authored-by: Jeremy Dorn <jeremy@jeremydorn.com>
Co-authored-by: Luke Sonnet <luke.sonnet@gmail.com>
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.

[Bug] Conversion Delay settings for Fact Tables Metrics working incorrectly
2 participants