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

feat(experiments): explore results in an insight #21235

Merged
merged 13 commits into from
Apr 11, 2024

Conversation

jurajmajerik
Copy link
Contributor

@jurajmajerik jurajmajerik commented Mar 28, 2024

Changes

Adds the ability to explore experiment results in an insight.

2024-03-28 22 48 47

To make sure we correctly query all participants we need to work with the exact start/end time of the experiment, as opposed to time rounded to start/end of day.

Unfortunately, the query doesn't manage to load when the hogql-insight-funnels flag is enabled - see below. @thmsobrmlr - can you please help us out?

2024-03-28 22 55 47

UI follow-ups:

  • Hide/uncheck the "Baseline" bar from the insight
  • Show a warning next to the timeframe picker to notify the user that the insight pertains to the exact start/end times of the experiment
image

How did you test this code?

👀

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

8 snapshot changes in total. 0 added, 8 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Mar 28, 2024

Size Change: 0 B

Total Size: 845 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 845 kB

compressed-size-action

@@ -624,6 +624,8 @@ export interface InsightsQueryBase extends Node {
aggregation_group_type_index?: integer
/** Sampling rate */
samplingFactor?: number | null

explicitDate?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Fly-by comment (off for easter, will have a deeper look next Tuesday) - I don't think we handle this setting in the new insights at all. Seems it gets passed in via the date range filter. I'll look into adding it and would appreciate any context :) After a first look I'd think this should belong into the dateRange object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't see the DateRange component before, but agree should go there!

For context, we need this setting whenever we don't want to round date ranges to start and end of day. This is particularly important for experiments because we want to discard all testing data that came before the exact moment the experiment began.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need this setting as well to move both trends & funnel experiments backend off of filters and onto query 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

For more context, the appropriate setting in filters is: https://github.com/PostHog/posthog/blob/master/posthog/schema.py#L2549

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

@jurajmajerik please take a look once.

I'll do the hiding baseline in a follow up (or maybe never :P ) because it needs to touch the insight internals quite a bit

@@ -32,7 +29,7 @@ export function ExperimentView(): JSX.Element {
<PageHeaderCustom />
<div className="space-y-8 experiment-view">
{experimentLoading ? (
<ExperimentLoader />
<LoadingState />
Copy link
Contributor

Choose a reason for hiding this comment

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

added a non-list based skeleton, looked nicer to me

const { experimentId, editingExistingExperiment, experimentMissing } = useValues(experimentLogic)

if (experimentMissing) {
return <NotFound object="experiment" />
Copy link
Contributor

Choose a reason for hiding this comment

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

handling not found states for new UI

@@ -284,7 +283,7 @@ export function SecondaryMetricsTable({

<div className="w-1/2 flex flex-col justify-end">
<div className="ml-auto">
{metrics && metrics.length > 0 && metrics.length < 3 && isExperimentRunning && (
{metrics && metrics.length > 0 && metrics.length < 3 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

On Draft mode you could only add one secondary metric 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

now extending this to stopped experiments as well

@neilkakkar
Copy link
Contributor

image

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

7 snapshot changes in total. 0 added, 7 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot posthog-bot removed the stale label Apr 11, 2024
@jurajmajerik
Copy link
Contributor Author

Nice! Great that you've added the Explore button to the empty state too 👍

@jurajmajerik jurajmajerik merged commit ed0abf2 into master Apr 11, 2024
105 checks passed
@jurajmajerik jurajmajerik deleted the experiment-explore-insight branch April 11, 2024 09:05
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.

4 participants