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

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

Merged
merged 3 commits into from Dec 6, 2023

Conversation

lukesonnet
Copy link
Contributor

Features and Changes

Users can currently choose to 'Exclude in-progress conversions' to drop users that have not had the full time window to convert. However, we don't properly calculate that "full time window". Currently, we just use the metric (or denominator metric if a funnel or ratio metric) conversion window, but we need to take:

  • the max of the numerator and denominator windows (for ratio metrics)
  • the sum of the numerator and recursive chain of denominators (for funnel metrics, since their conversion windows cascade)

We also need to always add the activation metric (if specified) window to the value of the above since activation metrics start their own conversion windows. So if a user has 72 hours to activate and a ratio with (24 hours numerator, 1 hours denominator), we need to give them a total of MAX(24, 1) + 72 hours to convert.

Closes #1844

Also, this followed work in #1880, which, while addressing some issues, didn't capture the full complexity of the above flow.

Testing

Added unit tests

Tested in-app:

  • Created 3 metrics
    • Activation metric with 72 hour conversion window
    • Plain metric with 24 hour conversion window
    • Ratio metric where denominator has 1 hour conversion window
    • Funnel metric where denominator has 1 hour conversion window
  • Compared SQL for running experiment for all three metrics with Exclude In-Progress Conversions

Results as expected:

  • Plain metric has a filter for 24 hours before experiment end
  • Ratio metric has a filter with MAX(24, 1)=24 hours before experiment end
  • Funnel metric has a filter with SUM(24, 1)=25 hours before experiment end

Adding the activation metric adds 72 hours to all of those above.\

Note: the metric filter in getMetricEnd remains a little inefficient because it doesn't know that it only needs the max for ratio metrics rather than the sum for funnel metrics, but it isn't dropping relevant data, just is collecting slightly too much data.

Copy link

github-actions bot commented Dec 5, 2023

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

Preview environment endpoints are available at:

@lukesonnet lukesonnet merged commit cbf94fa into main Dec 6, 2023
3 checks passed
@lukesonnet lukesonnet deleted the ls/fix-conversion-max branch December 6, 2023 16:36
bryce-fitzsimons pushed a commit that referenced this pull request Dec 12, 2023
…1921)

* Modify cascading window logic for excluding in-progress conversions

* Move fn

* Test
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] Incorrect "In Progress Conversions" behavior with ratio metrics that have different conversion windows
2 participants