feat(insights): Log scale option in Trends #22995

nikitaevg · 2024-06-16T10:17:03Z

Problem

#22564

Changes

Add support for log scale for trends.

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

Yes

How did you test this code?

Manually + a storybook test

nikitaevg

@pauldambra please take a look (or reroute to somebody?)

About tests, the only proper way I see to test this is a visual test. It turns out visual tests don't draw the graphs (see this for example), so I actually don't see any good way to test this 🤷

I also wonder if I should keep the y axis tick values, or remove them, like this.

nikitaevg · 2024-06-16T10:54:17Z

frontend/src/queries/nodes/InsightQuery/utils/legacy.ts

@@ -16,6 +16,7 @@ export const isLegacyTrendsFilter = (filters: Record<string, any> | undefined):
        'show_labels_on_series',
        'compare',
        'compare_to',
+        'y_axis_scale_type',


I wonder if this part is needed? What is the legacy trends, are they still present?

CC @thmsobrmlr

(Thomas will be back on Monday, so will wait with merging this till then)

It's not necessary to add here. This is a frontend side migration for notebook nodes (these already store "queries", whereas insights store "filters"). Legacy in this case refers to some properties that had been snake_cased and are now camelCased.

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

nikitaevg · 2024-06-16T12:00:05Z

frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx

                        callback: (value) => {
                            return formatPercentStackAxisValue(trendsFilter, value, isPercentStackView)
                        },
                    },
                    grid: gridOptions,
+                    type: yAxisScaleType === 'log10' ? 'logarithmic' : 'linear',


Log scale graphs don't show zero values, see the screenshot. This can be fixed (as per this thread), I wonder if it's needed?

Hmm, I think complete omission – as in the screenshot – is pretty confusing. But Grafana seems to do this pretty well! They seem to treat 0 values as essentially negative infinite on the log scale:

Can we pull this off?

The only workaround I found that might work is

Replace zeroes with 1e-12

Set minimum of the graph to the minimum value among the trend values (so that the graph doesn't show 1e-12, like here)

Figure out how to show "0" instead of "1e-12" when hovering, this person couldn't do it.

Do you think it's worth trying? Because that sounds like a lot and the result is not guaranteed.

Played with this a bit and got Grafana-like results (pushed a commit):

Before

After

Damn, thanks!

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

ee/api/test/test_dashboard_collaborators.py

Twixes

Looks great to me, just got a suggestion about the zeros!

As for the schema handling, I'll tag @thmsobrmlr here, since he knows much better about the cleaning/conversion functions we've got

Twixes

Dug into Chart.js a bit, the chart feels pretty smooth now 😄

Twixes · 2024-06-20T12:14:38Z

frontend/src/types.ts

@@ -2159,6 +2159,7 @@ export interface TrendsFilterType extends FilterType {
    show_values_on_series?: boolean
    show_labels_on_series?: boolean
    show_percent_stack_view?: boolean
+    y_axis_scale_type?: string // "log10" or "linear"


Maybe

Suggested change

y_axis_scale_type?: string // "log10" or "linear"

y_axis_scale_type?: 'log10' | 'linear'

@thmsobrmlr?

Oh, makes sense, updated, thanks!

Twixes · 2024-06-20T12:15:05Z

frontend/src/queries/nodes/InsightQuery/utils/legacy.ts

@@ -16,6 +16,7 @@ export const isLegacyTrendsFilter = (filters: Record<string, any> | undefined):
        'show_labels_on_series',
        'compare',
        'compare_to',
+        'y_axis_scale_type',


CC @thmsobrmlr

nikitaevg · 2024-06-20T17:21:23Z

frontend/src/scenes/insights/views/LineGraph/LineGraph.tsx

                        ...tickOptions,
-                        precision,
+                        display: !hideYAxis,
+                        ...(yAxisScaleType !== 'log10' && { precision }), // Precision is not supported for the log scale


I can't leave precision as is here because types don't match then, because precision is not a field for log scale.

Oh of course, I didn't notice TypeScript alerting me about this in VS Code, so missed that

# Conflicts: # frontend/src/queries/nodes/InsightQuery/utils/queryNodeToFilter.test.ts # frontend/src/queries/nodes/InsightQuery/utils/queryNodeToFilter.ts # frontend/src/queries/nodes/InsightViz/utils.ts # frontend/src/queries/schema.ts # frontend/src/scenes/insights/insightVizDataLogic.ts # frontend/src/scenes/insights/utils/compareInsightQuery.ts

thmsobrmlr

Great work!

thmsobrmlr · 2024-06-27T12:25:54Z

frontend/src/queries/nodes/InsightQuery/utils/legacy.ts

@@ -16,6 +16,7 @@ export const isLegacyTrendsFilter = (filters: Record<string, any> | undefined):
        'show_labels_on_series',
        'compare',
        'compare_to',
+        'y_axis_scale_type',


It's not necessary to add here. This is a frontend side migration for notebook nodes (these already store "queries", whereas insights store "filters"). Legacy in this case refers to some properties that had been snake_cased and are now camelCased.

nikitaevg added 3 commits June 16, 2024 11:13

feat(insights): support log scale

d80cabf

fix types

5350249

revert storybook tests

d2bb9c8

nikitaevg commented Jun 16, 2024

View reviewed changes

nikitaevg marked this pull request as ready for review June 16, 2024 12:05

pauldambra requested a review from a team June 16, 2024 12:24

pauldambra reviewed Jun 16, 2024

View reviewed changes

frontend/src/scenes/insights/EditorFilters/YAxisScaleSettings.tsx Outdated Show resolved Hide resolved

nikitaevg added 2 commits June 16, 2024 13:56

Merge remote-tracking branch 'upstream/master' into 22564-log-scale

20ff6ca

fix a flaking test

c85d199

nikitaevg commented Jun 16, 2024

View reviewed changes

ee/api/test/test_dashboard_collaborators.py Show resolved Hide resolved

Turn the scale picker into a segmented button

87b7a7b

Twixes requested changes Jun 19, 2024

View reviewed changes

Twixes requested a review from thmsobrmlr June 19, 2024 13:34

Twixes changed the title ~~feat(insights): support log scale~~ feat(insights): Log scale option in Trends Jun 19, 2024

uncomment forgotten piece

fe7f9b6

nikitaevg requested a review from Twixes June 19, 2024 18:31

Show zeros in log scale

852df2d

Twixes approved these changes Jun 20, 2024

View reviewed changes

Twixes mentioned this pull request Jun 20, 2024

feat(insights): Support unary minus and plus in formulas #23119

Merged

change field type to enum

4661d35

nikitaevg commented Jun 20, 2024

View reviewed changes

nikitaevg and others added 6 commits June 20, 2024 18:28

fix typo

7d013d4

update the schema for handlers

bce8552

oh, I can generate schemas

4c330af

Merge branch 'master' into 22564-log-scale

7066480

remove from notebook migration

05ccb02

thmsobrmlr approved these changes Jun 27, 2024

View reviewed changes

thmsobrmlr merged commit 6144971 into PostHog:master Jun 27, 2024
88 of 90 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(insights): Log scale option in Trends #22995

feat(insights): Log scale option in Trends #22995

nikitaevg commented Jun 16, 2024 •

edited

Loading

nikitaevg left a comment •

edited

Loading

nikitaevg Jun 16, 2024

Twixes Jun 20, 2024

Twixes Jun 20, 2024

thmsobrmlr Jun 27, 2024

nikitaevg Jun 16, 2024

Twixes Jun 19, 2024

nikitaevg Jun 19, 2024

Twixes Jun 20, 2024

nikitaevg Jun 20, 2024

Twixes left a comment

Twixes left a comment

Twixes Jun 20, 2024

nikitaevg Jun 20, 2024

Twixes Jun 20, 2024

nikitaevg Jun 20, 2024

Twixes Jun 20, 2024

thmsobrmlr left a comment

thmsobrmlr Jun 27, 2024

	y_axis_scale_type?: string // "log10" or "linear"
	y_axis_scale_type?: 'log10' \| 'linear'

feat(insights): Log scale option in Trends #22995

feat(insights): Log scale option in Trends #22995

Conversation

nikitaevg commented Jun 16, 2024 • edited Loading

Problem

Changes

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

How did you test this code?

nikitaevg left a comment • 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

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

Before

After

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

Twixes left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thmsobrmlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitaevg commented Jun 16, 2024 •

edited

Loading

nikitaevg left a comment •

edited

Loading