feat: Add goal lines to trends + smarter default alerts #28461

rafaeelaudibert · 2025-02-07T20:52:22Z

Problem

When creating an insight from Web Vitals we are copying the thresholds over but they are not editable. Similarly, we'd love if people could easily set up alerts for web vitals with sane values.

Changes

Goal lines can be added to Trends insights!
Alerts are smarter on creation, they'll inherit the smallest goal line value

https://www.loom.com/share/442e793b77694b8bb66e871bfb084c24?sid=5e7a71bc-5161-473a-b36f-1f4ba113e76f

Does this work well for both Cloud and self-hosted?

Yes, sir

How did you test this code?

Manually

rafaeelaudibert · 2025-02-07T20:53:42Z

@mariusandra Pinging you because you're the kea resident. If you look at my video you'll see we're querying every time I change a value in the goal line input (including the label name). Any suggestion on how we could delay updating the querySource to avoid the query going out so often?

greptile-apps

PR Summary

This PR adds goal lines functionality to trends insights and improves alert creation by inheriting threshold values from goal lines. Here's a concise summary of the key changes:

Added new goal lines UI component in DisplayTab.tsx with label visibility controls and value inputs
Implemented goal line visualization in LineGraph.tsx using chart.js annotations with hover interactions
Enhanced alert creation logic in alertFormLogic.ts to use smallest goal line value as default threshold
Added new goalLinesLogic.ts to manage goal line state and operations
Integrated goal lines with existing trends insights and web vitals functionality

Key points to note:

Added proper type handling and validation for goal line values
Implemented synchronized state between goal lines and chart settings
Enhanced alert modal to access insight data through insightLogicProps
Added support for goal line display on both left and right Y-axes
Improved visual styling with consistent line-height for series glyphs

_{11 file(s) reviewed, 11 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

frontend/src/queries/nodes/DataVisualization/Components/DisplayTab.tsx

frontend/src/lib/components/Alerts/alertFormLogic.ts

frontend/src/queries/nodes/InsightViz/EditorFilters.tsx

frontend/src/scenes/insights/EditorFilters/GoalLines.tsx

frontend/src/scenes/insights/InsightPageHeader.tsx

frontend/src/scenes/insights/EditorFilters/goalLinesLogic.ts

posthog-bot · 2025-02-07T22:02:16Z

📸 UI snapshots have been updated

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

chromium: 0 added, 32 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 30 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

github-actions · 2025-02-07T22:15:15Z

Size Change: 0 B

Total Size: 1.21 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	1.21 MB

_{compressed-size-action}

mariusandra · 2025-02-10T11:37:50Z

@mariusandra Pinging you because you're the kea resident. If you look at my video you'll see we're querying every time I change a value in the goal line input (including the label name). Any suggestion on how we could delay updating the querySource to avoid the query going out so often?

I think the quickest win you can do is to update the value onBlur, instead of onChange. However even then we should not need to make a request. Sadly I don't know what the current insightLogic, caching and reloading situation is. You'll have to ask someone from Product Analytics.

In the olden days, I had a dream, where a query would look like this:

const query: InsightVizNode = {
    kind: NodeKind.InsightVizNode,
    source: {
        kind: NodeKind.TrendsQuery,
        series: [
            {
                event: '$pageview',
                name: '$pageview',
                kind: NodeKind.EventsNode,
                math: BaseMathType.UniqueUsers,
            },
        ],
        breakdownFilter: {
            breakdown: `$feature/${featureFlag.key}`,
            breakdown_type: 'event',
        },
    },
}

and in there, the TrendsQuery would contain everything that needed a server reload, whereas all "client side" attributes would be added on the InsightVizNode. We'd probably make a custom TrendsVizNode for all the attributes.

Thus this code:

actions.updateQuerySource({
    trendsFilter: {
        ...querySource?.trendsFilter,
        goalLines: values.goalLines,
    },
} as Partial<TrendsQuery>)

would become:

actions.updateQuery({
    goalLines: values.goalLines,
    ...query
} as Partial<TrendsVizNode>)

However I believe such a distinction is not made today... and I'm not sure if there's anything in place to say "shouldn't reload".

rafaeelaudibert · 2025-02-10T12:31:20Z

I think the quickest win you can do is to update the value onBlur, instead of onChange. However even then we should not need to make a request. Sadly I don't know what the current insightLogic, caching and reloading situation is. You'll have to ask someone from Product Analytics.

Tried that, but it doesn't work because our LemonInputs is controllable. Having a local useState for those values is very contrived because we've got a list of goal lines. I believe I'll keep it like this for now, and we can come back and optimize this eventually.

and in there, the TrendsQuery would contain everything that needed a server reload, whereas all "client side" attributes would be added on the InsightVizNode. We'd probably make a custom TrendsVizNode for all the attributes.
However I believe such a distinction is not made today... and I'm not sure if there's anything in place to say "shouldn't reload".

Oh, absolutely. The catch here is that the backend needs the trendsFilter values to decide how it should group the values (area vs. line, for example) but we're reusing it for way too much stuff, including goalLines which aren't really a requirement in the backend - it's not limited to my addition, though, some other fields aren't used in the backend but are being passed in.

robbie-c · 2025-02-10T14:20:34Z

I'll let @PostHog/team-product-analytics handle this, but if it gets to weds without a review I'll take another look :)

posthog-bot · 2025-02-11T13:33:57Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

thmsobrmlr · 2025-02-12T10:12:07Z

@mariusandra Pinging you because you're the kea resident. If you look at my video you'll see we're querying every time I change a value in the goal line input (including the label name). Any suggestion on how we could delay updating the querySource to avoid the query going out so often?

You can add the goalLines key to the properties here https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/insights/utils/queryUtils.ts#L150. This will ignore it when deciding if a new query should be made to the backend. Other than that you could add a debounce to the input, though up to you to decide if that feels nicer than the instantaneous moving of the goal line.

thmsobrmlr

Great work @rafaeelaudibert !

Changes look good to me code wise, aside from a question I put inline. I'm only a bit worried about further complicating the insights UI, as people are already overwhelmed by it now. Also it moves the chart further down and it's now completely below the fold on a laptop. What about moving the goal lines to a button next to the alerts one?

IMO we could even move both into the "..."-menu at the top in a separate section. We wanted to have alerts more accessible in the beginning to help feature adoption though.

thmsobrmlr · 2025-02-12T09:58:39Z

frontend/src/queries/nodes/InsightViz/EditorFilters.tsx

-    )
-
-    editorFilterGroups = [
+    const editorFilterGroups: InsightEditorFilterGroup[] = [


Why this change? Having the distinction between left/right filters shows the intent of the code.

I was just removing the unnecessary let, but I can revert this, it doesn't impact anything besides my OCD urge to change it :)

rafaeelaudibert · 2025-02-12T17:22:32Z

@thmsobrmlr

You can add the goalLines key to the properties here master/frontend/src/scenes/insights/utils/queryUtils.ts#L150. This will ignore it when deciding if a new query should be made to the backend. Other than that you could add a debounce to the input, though up to you to decide if that feels nicer than the instantaneous moving of the goal line.

Ah, that's great. It's not easy to find, but it definitely does the job. I'll keep it without debouncing for now.

I'm only a bit worried about further complicating the insights UI, as people are already overwhelmed by it now. Also it moves the chart further down and it's now completely below the fold on a laptop. What about moving the goal lines to a button next to the alerts one?

IMO we could even move both into the "..."-menu at the top in a separate section. We wanted to have alerts more accessible in the beginning to help feature adoption though.

Ah, interesting. It appears to the left for me because of my screen size I suppose? I'll see what I can do here, might simplify the UI a little. I don't want to put it at the top because this is customizing the query, while Alerts is something extraneous to the query, so it makes sense to live somewhere else.

rafaeelaudibert · 2025-02-12T17:29:37Z

@thmsobrmlr https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/insights/utils/queryUtils.ts#L129 mentions we need to keep the backend in sync with that code. It's linking to the wrong place, but I'll assume it's meant to link to this:

It's not up-to-date anymore, would you like me to fix it, or is it intended?

rafaeelaudibert · 2025-02-12T22:41:22Z

@thmsobrmlr I've hidden the goal lines + some existing settings behind a dropdown. You can see that in the new snapshots - or test locally, of course. Do you think this is mergeable now?

posthog-bot · 2025-02-13T14:12:06Z

📸 UI snapshots have been updated

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

chromium: 0 added, 54 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 52 modified, 0 deleted (diff for shard 2)

Triggered by this commit.

👉 Review this PR's diff of snapshots.

robbie-c · 2025-02-13T14:49:47Z

frontend/__snapshots__/scenes-app-insights--funnel-historical-trends-edit--light.png

IMO the filters section from the older version is better. I understand hiding the advanced options behind a dropdown but what's the rationale for including it here?

robbie-c · 2025-02-13T14:57:54Z

frontend/__snapshots__/scenes-app-insights--trends-number-edit--dark.png

What does it mean to have a goal line on a Number?

robbie-c · 2025-02-13T15:00:48Z

frontend/src/queries/nodes/InsightViz/PropertyGroupFilters/PropertyGroupFilters.tsx

-                        query={query}
-                        setQuery={setQuery as (node: InsightQueryNode) => void}
-                    />
+                    <div className="flex flex-1 gap-2 flex-row space-between">


Ah yeah - I commented on this screenshot.

What's the rationale for this change? In the screenshot examples it forces the label for InsightTestAccountFilter onto two lines which IMO looks weird

Saving some real estate space. It'll only wrap into two lines in smaller screens and it's still more space than we had before. Thomas' major complaint was that we were using too much space and the chart was under the fold for modern small laptops so this was my attempt to improve that slightly. Happy to revert if you aren't happy with the result.

I will defer to @thmsobrmlr and @PostHog/team-product-analytics on this :)

posthog-bot · 2025-02-13T17:56:17Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Let's make it two colors to improve visibility

These already existed and were being used by Web Vitals, but they're now available to be edited in the UI

It's common that people will create goal lines as an indication of whether they're above the value they want to be. Because of that, when setting up a new alert from the insights page we'll now use the smallest goal line as the default value for the alert

In some smaller laptops the chart is under the fold which isn't ideal. I've moved a lot of the options under an "advanced options" dropdown. I've included the new goal lines but also some existing settings that don't need to be toggled often. I've revamped the types as well because this is was extremely outdated. There's no more concept of left/right, nor anything to do with count.

I had a feature flag locally that messed up my visualization. I've now removed it and I can clearly see I messed up in my most recent commit

posthog-bot · 2025-02-14T19:20:04Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

thmsobrmlr

Great work @rafaeelaudibert! Lot's of improvements in here.

There is one thing blocking this from merging: The flag for determining wether the GoalLines filter should be displayed doesn't work reliably. For example the filter isn't present for a new insight. This is because the flag doesn't take defaults into account. I trust you to fix this though and am handing out a ✅ already.

Ah, interesting. It appears to the left for me because of my screen size I suppose? I'll see what I can do here, might simplify the UI a little. I don't want to put it at the top because this is customizing the query, while Alerts is something extraneous to the query, so it makes sense to live somewhere else.

Oh, I see! You seem to have feature flag insight-horizontal-controls enabled, likely due to syncing all feature flags. That flag is an experiment and currently not rolled out to users - they'll see the version with the controls on top (just funnels are horizontal by default). The horizontal layout is also used for very small (mobile) screen sizes.

@thmsobrmlr master/frontend/src/scenes/insights/utils/queryUtils.ts#L129 mentions we need to keep the backend in sync with that code. It's linking to the wrong place, but I'll assume it's meant to link to this:

It's not up-to-date anymore, would you like me to fix it, or is it intended?

Yup, they seem to have gotten out of sync. Feel free to fix it, or also just merge this PR and I'll do that separately.

What's the rationale for this change? In the screenshot examples it forces the label for InsightTestAccountFilter onto two lines which IMO looks weird.

I think the filters look better in the left-to-right layout (most insights), but don't look good for the funnels insight. Would be good to have the original layout there. Not a blocker for merging though.

thmsobrmlr · 2025-02-14T20:23:19Z

frontend/src/queries/nodes/WebVitals/WebVitalsProgressBar.tsx

@@ -28,9 +28,9 @@ export function WebVitalsProgressBar({ value, metric }: WebVitalsProgressBarProp
                // eslint-disable-next-line react/forbid-dom-props
                style={{
                    width: `${goodWidth}%`,
-                    backgroundColor: band === 'good' ? WEB_VITALS_COLORS.good : undefined,
+                    backgroundColor: band === 'good' ? `${WEB_VITALS_COLORS.good} !important` : undefined,


I'm not quite sure why these changes are included in this PR, but I trust you to verify whether they are intentional.

Yep, that was a fly-by

thmsobrmlr · 2025-02-14T20:52:40Z

Didn't see that automatic merging is enabled @rafaeelaudibert and don't think the issues I mentioned warrant a revert. Would be great if you can follow up with a fix though.

rafaeelaudibert · 2025-02-14T21:02:04Z

@thmsobrmlr Oh yikes, my bad with the automerge. I'll follow up with the small fixes :). Thank you!

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

rafaeelaudibert added the team/web-analytics label Feb 7, 2025

rafaeelaudibert requested review from a team February 7, 2025 20:52

greptile-apps bot reviewed Feb 7, 2025

View reviewed changes

rafaeelaudibert force-pushed the improved-web-vitals-goal-tracking-plus-alerts branch from f02641f to 0b9016e Compare February 7, 2025 21:40

PostHog deleted a comment from posthog-bot Feb 7, 2025

rafaeelaudibert requested a review from robbie-c February 10, 2025 12:21

rafaeelaudibert force-pushed the improved-web-vitals-goal-tracking-plus-alerts branch from d992149 to c0747c1 Compare February 11, 2025 13:13

rafaeelaudibert enabled auto-merge (squash) February 11, 2025 21:13

thmsobrmlr requested changes Feb 12, 2025

View reviewed changes

rafaeelaudibert force-pushed the improved-web-vitals-goal-tracking-plus-alerts branch 2 times, most recently from 3e4717f to b2ca5a5 Compare February 12, 2025 21:06

rafaeelaudibert requested a review from thmsobrmlr February 12, 2025 22:37

rafaeelaudibert force-pushed the improved-web-vitals-goal-tracking-plus-alerts branch from 9333ad7 to dc9bbdd Compare February 13, 2025 13:49

PostHog deleted a comment from posthog-bot Feb 13, 2025

robbie-c reviewed Feb 13, 2025

View reviewed changes

frontend/__snapshots__/scenes-app-insights--trends-number-edit--dark.png Outdated

Copy link

Member

robbie-c Feb 13, 2025

Choose a reason for hiding this comment

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

What does it mean to have a goal line on a Number?

robbie-c reviewed Feb 13, 2025

View reviewed changes

rafaeelaudibert and others added 16 commits February 14, 2025 15:59

ui: Fix web vitals card color

50b9291

Let's make it two colors to improve visibility

feat: Allow goal lines to be displayed/hidden on data visualization

a0075f1

feat: Add goal lines to Trends queries

cab8e44

These already existed and were being used by Web Vitals, but they're now available to be edited in the UI

fix: Display goal lines on trends only

595bc41

fix

5e965ef

types: Fix unreasonable TS error

193a4a2

fix: Stop rerendering on goalLines changes

36c123c

fix: Fix web vitals colors

1f91b96

fix: Revert back to left/right differentiation

088cb6c

I had a feature flag locally that messed up my visualization. I've now removed it and I can clearly see I messed up in my most recent commit

Update UI snapshots for webkit (2)

651fdc0

Update UI snapshots for chromium (2)

017c588

fix: Display goal lines on line graphs only

52aa36d

Update UI snapshots for webkit (2)

9e2931a

Update UI snapshots for chromium (2)

194c665

rafaeelaudibert force-pushed the improved-web-vitals-goal-tracking-plus-alerts branch from 70e28b4 to 194c665 Compare February 14, 2025 18:59

github-actions bot added 2 commits February 14, 2025 19:17

Update UI snapshots for chromium (1)

210f8d7

Update UI snapshots for chromium (2)

3ed881e

thmsobrmlr approved these changes Feb 14, 2025

View reviewed changes

rafaeelaudibert merged commit ad159f5 into master Feb 14, 2025
104 checks passed

rafaeelaudibert deleted the improved-web-vitals-goal-tracking-plus-alerts branch February 14, 2025 20:49

rafaeelaudibert mentioned this pull request Feb 14, 2025

refactor: Small fixes to product analytics goals + layout #28754

Merged

anirudhpillai mentioned this pull request Feb 17, 2025

fix(alerts): navigation from alerts page #28774

Merged

HamedMP pushed a commit that referenced this pull request Feb 19, 2025

feat: Add goal lines to trends + smarter default alerts (#28461)

4788c8d

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add goal lines to trends + smarter default alerts #28461

feat: Add goal lines to trends + smarter default alerts #28461

rafaeelaudibert commented Feb 7, 2025 •

edited

Loading

rafaeelaudibert commented Feb 7, 2025

greptile-apps bot left a comment

posthog-bot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025 •

edited

Loading

mariusandra commented Feb 10, 2025

rafaeelaudibert commented Feb 10, 2025 •

edited

Loading

robbie-c commented Feb 10, 2025

posthog-bot commented Feb 11, 2025

thmsobrmlr commented Feb 12, 2025

thmsobrmlr left a comment

thmsobrmlr Feb 12, 2025

rafaeelaudibert Feb 12, 2025

rafaeelaudibert commented Feb 12, 2025

rafaeelaudibert commented Feb 12, 2025

rafaeelaudibert commented Feb 12, 2025

posthog-bot commented Feb 13, 2025

robbie-c Feb 13, 2025 •

edited

Loading

robbie-c Feb 13, 2025

robbie-c Feb 13, 2025

rafaeelaudibert Feb 13, 2025

robbie-c Feb 13, 2025

posthog-bot commented Feb 13, 2025

posthog-bot commented Feb 14, 2025

thmsobrmlr left a comment

thmsobrmlr Feb 14, 2025

rafaeelaudibert Feb 14, 2025

thmsobrmlr commented Feb 14, 2025

rafaeelaudibert commented Feb 14, 2025

feat: Add goal lines to trends + smarter default alerts #28461

feat: Add goal lines to trends + smarter default alerts #28461

Conversation

rafaeelaudibert commented Feb 7, 2025 • edited Loading

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

rafaeelaudibert commented Feb 7, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

posthog-bot commented Feb 7, 2025

📸 UI snapshots have been updated

github-actions bot commented Feb 7, 2025 • edited Loading

mariusandra commented Feb 10, 2025

rafaeelaudibert commented Feb 10, 2025 • edited Loading

robbie-c commented Feb 10, 2025

posthog-bot commented Feb 11, 2025

📸 UI snapshots have been updated

thmsobrmlr commented Feb 12, 2025

thmsobrmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaeelaudibert commented Feb 12, 2025

rafaeelaudibert commented Feb 12, 2025

rafaeelaudibert commented Feb 12, 2025

posthog-bot commented Feb 13, 2025

📸 UI snapshots have been updated

robbie-c Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posthog-bot commented Feb 13, 2025

📸 UI snapshots have been updated

posthog-bot commented Feb 14, 2025

📸 UI snapshots have been updated

thmsobrmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thmsobrmlr commented Feb 14, 2025

rafaeelaudibert commented Feb 14, 2025

rafaeelaudibert commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

rafaeelaudibert commented Feb 10, 2025 •

edited

Loading

robbie-c Feb 13, 2025 •

edited

Loading