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

[stats] Add support for daily and cumulative time series to integrations and gbstats #1334

Merged
merged 53 commits into from
Jun 26, 2023

Conversation

lukesonnet
Copy link
Collaborator

@lukesonnet lukesonnet commented Jun 6, 2023

Features and Changes

  • Adds support for cumulative and daily time series, but does not expose them to GrowthBook users yet.
  • Also refactors our general query approach to rely on __userMetric as our main dataset with all user and metric data (LEFT JOIN metrics on to users rather than an inner join)

Overview

Adds ability to create cumulative and daily time series.

Our current time series just are dimension results by user's first date bucketed. So that means on day X, we see experimental effects for all users bucketed on day X, regardless of whether those conversions happened on day X or day X+1 and so on. Furthermore, on day X, we don't have any users that were bucketed on days X-1 and backwards influencing the metric, so it doesn't tell you what happened on day X but rather what happened to users bucketed on day X.

There's ample demand for Cumulative Time Series, which show you the total effects for all users and all metrics through day X. In other words, what is the experiment result as if I had run my results at the end of day X?

We also might care about Daily Time Series, which show you for all users bucketed through day X, what is the effect on metrics that were "accrued" on day X. This probably only matters for those using the Experiment Duration attribution model or long conversion windows, but can let you know what is happening on given days in your experimental population as the experiment goes on. This is effectively the derivative of the Cumulative Time Series, but with appropriate statistics rather than simply diff'ing results in the front end.

This PR adds the necessary components to gbstats and our SQL integrations to compute these last two series: Cumulative and Daily Time Series.

How?

  • UsesgetDateTable to build a time series of dates and then cross joining user metric results to that before aggregating. This method is a generic method that should work across all SQL engines and is very cheap. Other methods, such as UNNESTing, may be more compact in their SQL but are slightly less performant and vary across engines.
  • Reworking the entire SQL query to focus on LEFT JOINing __metric to __distinctUsers to rely on this as our main count of users and to left join other auxiliary metrics on to (e.g. denominator and/or CUPED metrics). This is also where we cross join with the above date range to build our time series.
  • Splits the __userMetric CTE into __userMetricJoin and __userMetricAgg so that custom aggregations can operate over a column of metric values that are COALESCE(value, 0) for users who have 1+ conversion and are NULL for users with 0 matching conversions.
  • In gbstats, then uses diff() to create the daily time series, which are just the metric values added from day to day. In that way, it tells you "what changed in the cumulative time series on this day" rather than some abstract redefinition of conversion windows, custom aggregations, etc. to fit neatly in one day, which is what we previously discussed.
  • Also incorporates FE changes from @bryce-fitzsimons that add tooltips and change some of the logic for our experiment date graphs.

Side effects:

  • Custom aggregations may behave somewhat differently
    • Custom aggregation gets applied to a value column we create that IS NULL for users with no matching rows in their conversion window, and is COALESCE(value, 0) otherwise. This could affect people that use AVG, COUNT or other operators, but allows us to have a clear distinction in the aggregation step between users that have no matching conversion and treats those with matching conversion but no values as 0s. This is explicit. I think this change is for the best, but it will alter some user behavior if they have null values in their data.
  • User counts for ratio metrics may change, but TBD

TODO:

  • Notebook generator
  • Merging with Bryce's front-end UI

Dependencies

n/a

Testing

  • Build front-end logic to control setting cumulativeDate (NOTE: THIS IS IN PLACE BUT NOT EXPOSED TO THE USER)
  • Clean up some potentially unnecessary casting and somewhat inconsistent SQL across engines

Closes GB-251

Dependencies

n/a

Testing

All integration test queries execute:

  • bigquery
  • clickhouse
  • mysql
  • postgres
  • presto
  • snowflake
  • mssql
  • redshift
  • athena (no testing set up)
  • databricks (no testing set up)

Cumulative time series on latest day is the same as the full results without any time series:

  • bigquery
  • clickhouse
  • mysql
  • postgres
  • presto
  • snowflake
  • mssql
  • redshift
  • athena (no testing set up)
  • databricks (no testing set up)

Cumulative time series on day X is the same as if we had run full results at midnight at the end of day X:

  • postgres
  • Will not test others, but will just check equivalence of time series results to postgres results on same data (DONE)

TODO:

  • Test that daily time series (built solely using diff()) produces correct results

Screenshots

Updated FE for cohort TS:
https://www.loom.com/share/4c014e60163b4c36840d883909f69ebc

Example SQL

Example queries run as part of the integration testing can be found in this branch here: https://github.com/growthbook/example-sql/tree/lsonnet/ts-left-twocteusermetric/queries

Some of particular interest:

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

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

Preview environment endpoints are available at:

@linear
Copy link

linear bot commented Jun 7, 2023

@lukesonnet lukesonnet changed the title WIPv2 [stats] Add support for daily and cumulative time series to integrations and gbstats [stats] Add support for daily and cumulative time series to integrations and gbstats Jun 7, 2023
@lukesonnet lukesonnet marked this pull request as ready for review June 7, 2023 20:44
@github-actions
Copy link

github-actions bot commented Jun 21, 2023

Deploy preview for docs ready!

✅ Preview
https://docs-433r5ep8c-growthbook.vercel.app

Built with commit 97f684d.
This pull request is being automatically deployed with vercel-action

.map((d) => `SELECT ${d} AS day`)
.join("\nUNION ALL\n");
return `
SELECT ${this.dateTrunc(this.castToDate("t.day"))} AS day
Copy link
Collaborator Author

@lukesonnet lukesonnet Jun 23, 2023

Choose a reason for hiding this comment

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

This may appear a little redundant, but dateTrunc doesn't work on strings in a lot of cases, and DATE sometimes returns time information and we just want to use the full date when comparing with the metric/assignment data.

@lukesonnet lukesonnet merged commit acd0a34 into main Jun 26, 2023
4 checks passed
@lukesonnet lukesonnet deleted the lsonnet/ts-left-twocteusermetric branch June 26, 2023 20:51
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.

None yet

3 participants