feat(experiments): Tag support for shared metrics #28061

danielbachhuber · 2025-01-29T21:19:01Z

From https://posthog.slack.com/archives/C07PXH2GTGV/p1738179059221599

the ability to create metric groups that you can add to experiments.

Changes

One or more tags can be added to a shared metric:

The tags are listed in the shared metrics table:

When your shared metrics have some tags, you can click on the tag to quickly select all matching metrics:

How did you test this code?

Manual review.

greptile-apps

PR Summary

This PR adds tag support for shared metrics in experiments, allowing users to organize and quickly filter metrics using tags in the UI.

Key changes:

Added experiment_saved_metric foreign key to TaggedItem model with corresponding database constraints and migrations
Integrated TaggedItemSerializerMixin into ExperimentSavedMetricSerializer to handle tag operations
Added tag UI components in SharedMetric.tsx for managing tags on individual metrics
Implemented quick-select functionality in SharedMetricModal.tsx to filter metrics by clicking tags
Added conditional tag column in shared metrics table based on TAGGING feature flag

The implementation follows existing patterns for tag management in PostHog and includes proper database constraints to maintain data integrity.

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

greptile-apps · 2025-01-29T21:19:47Z

frontend/src/scenes/experiments/Metrics/SharedMetricModal.tsx

+    const availableTags = Array.from(
+        new Set(
+            availableSharedMetrics
+                .filter((metric: SharedMetric) => metric.tags)
+                .flatMap((metric: SharedMetric) => metric.tags)
+                .filter(Boolean)
+        )
+    ).sort()


style: Consider memoizing availableTags with useMemo to prevent recalculation on every render

greptile-apps · 2025-01-29T21:19:49Z

frontend/src/scenes/experiments/SharedMetrics/SharedMetric.tsx

+                                    tags: newTags,
+                                })
+                            }}
+                            saving={false}


style: saving prop is hardcoded to false - should reflect actual saving state from sharedMetricLogic to show proper loading state

github-actions · 2025-01-29T21:31:23Z

Size Change: +756 B (+0.06%)

Total Size: 1.16 MB

ℹ️ View Unchanged

Filename	Size	Change
`frontend/dist/toolbar.js`	1.16 MB	+756 B (+0.06%)

_{compressed-size-action}

raquelmsmith · 2025-01-29T21:37:42Z

Nice love the speed on this! TY ❤️

I think there's opportunity for better UX for this, as this feels kinda "slapped on."

An example I thought of was Google Drive's folder thing to move a document - but instead of moving you are selecting the tag (which shouldn't be so obviously a tag, maybe?)

So...

Click Add primary/secondary metric
Select "shared metric"
See just a list of collections (folders in the Google Drive example)
Select the collection (which is a tag in the db but called "collection" or something here)
Can either just "select all" or "select some" - if "select some" they can then see all the individual metrics and manually uncheck whatever they don't want
What if they want to now add another metric that wasn't in that collection? Maybe they just go through the flow again for now, but it'd be nice to be able to create a new one on the fly eventually, or select another one from a different list.

Thoughts?

danielbachhuber · 2025-01-29T21:52:37Z

An example I thought of was Google Drive's folder thing to move a document - but instead of moving you are selecting the tag (which shouldn't be so obviously a tag, maybe?)

[...]

Thoughts?

Could, not strongly against. However, Google Docs doesn't let you select some docs from folder A and some docs from folder B:

CleanShot.2025-01-29.at.13.44.06.mp4

If we implemented such an ability, it would be difficult to see what's selected in folder A when you're in folder B. We could then add some UI that shows the selected metrics, but I think that's veering into overly complex territory.

I think there's opportunity for better UX for this, as this feels kinda "slapped on."

To help me better understand your perspective, what makes you feel like it's "slapped on"? Are there small things we could do to make it feel more polished?

To help inform the decision: there's currently one team with 28 metrics. Everyone else has 10 or less.

SELECT team_id, COUNT(*) AS row_count
FROM postgres_posthog_experimentsavedmetric
GROUP BY team_id ORDER BY row_count DESC

posthog-bot · 2025-01-29T21:57:55Z

📸 UI snapshots have been updated

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

chromium: 0 added, 2 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.

raquelmsmith · 2025-01-29T22:16:47Z

Yeah I think selecting metrics from a single collection for now is fine. If they want metrics from another collection they just go through the flow again.

what makes you feel like it's "slapped on"?

I think it's that in my mental model a "tag" is just metadata on the thing - it's a lower or secondary level of importance.

Whereas with a folder structure, you're making a primary decision about where something belongs and what it means.

I think it's good that we use our server implementation of tags for this, as it's existing and allows us to assign metrics to multiple "collections." But the folder mental model is better for this IMO.

Are there small things we could do to make it feel more polished?

I think it could be as simple as renaming "tag" to "collection", giving the names more space (tags feel like they should just be a single word, but I assume a collection will be multiple words).

Like if you were designing this from scratch, how would you do it? Probably more of the folder structure like google drive. And then we say "what existing tech do we have to make this work?"

These are all just my opinions, and getting something out soon is better than fussing about the ideal solution forever 😄 So feel free to move forward with what you think is best here!

posthog-bot · 2025-01-29T22:40:46Z

📸 UI snapshots have been updated

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

chromium: 0 added, 2 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 · 2025-01-29T23:20:16Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-01-30T14:07:22Z

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2025-01-30T14:27:35Z

📸 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 1)
webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

jurajmajerik · 2025-01-30T15:43:20Z

I'm quite happy with how this works, with the caveat that we should call it Collections, as Raquel suggested. Another thing to consider is that tagging is a Teams/Enterprise feature, which means it will trigger a paywall if you don’t have access to it. That seems fine to me, as I’d expect larger customers to need this - just wanted to surface it here.

danielbachhuber · 2025-01-30T15:52:23Z

with the caveat that we should call it Collections, as Raquel suggested.

Tell me more? How is this "Collections" concept different than tags?

Another thing to consider is that tagging is a Teams/Enterprise feature, which means it will trigger a paywall if you don’t have access to it. That seems fine to me, as I’d expect larger customers to need this - just wanted to surface it here.

Yep, this is consistent with dashboards, feature flags, actions, insights, event definitions, and property definitions.

jurajmajerik

Tell me more? How is this "Collections" concept different than tags?

I was thinking of renaming it in the UI to make it clearer that this is for grouping metrics into collections. But I realize that would make it inconsistent with the rest of the app, where tags are used.

Good to go as is 👍

danielbachhuber · 2025-01-30T21:25:48Z

posthog/migrations/0557_add_tags_to_experiment_saved_metrics.py

@@ -0,0 +1,92 @@
+# Generated by Django 4.2.18 on 2025-01-30 17:45


RIP easy to read migration: https://github.com/PostHog/posthog/blob/f34e0d4647458306e4e91c82934d982c7f12f0ca/posthog/migrations/0557_add_tags_to_experiment_saved_metrics.py

danielbachhuber requested review from raquelmsmith and a team January 29, 2025 21:19

greptile-apps bot reviewed Jan 29, 2025

View reviewed changes

danielbachhuber and others added 13 commits January 29, 2025 17:35

Tag support for shared metrics

6ac68ad

Memoize

e1ce7c3

Update query snapshots

99207c8

Update UI snapshots for chromium (2)

7cd8741

See if this unbreaks CI

8d8c0a6

More precise import

b4e327b

Ditch memoization; unnecessary complexity

c31ffa5

Rename migration

59cc36d

Fix type issues

ca82cd1

Update UI snapshots for chromium (2)

2543f93

Attempt to fix CI again

b2ef2df

Update UI snapshots for chromium (1)

46b64d2

Restore original migration

889f548

danielbachhuber force-pushed the experiments/tagged-shared-metrics branch from 487e8c9 to 889f548 Compare January 30, 2025 01:35

danielbachhuber and others added 6 commits January 30, 2025 03:56

Try a simplified migration

203011d

Production only has a check constraint, not a unique constraint

d25c407

Try nullifying unique_together

dab9062

Use <LemonButton> to make it look more like a button

a6af645

Filter out dupes

7b9c05d

Update UI snapshots for chromium (1)

ef9590c

Update UI snapshots for chromium (1)

2b5420f

Try modifying Django's state directly

a0835ea

danielbachhuber added 8 commits January 30, 2025 08:01

Use AddConstraintNotValid() to avoid locking the table

bc6572a

Merge branch 'master' into experiments/tagged-shared-metrics

11e1b71

Merge branch 'master' into experiments/tagged-shared-metrics

1cec059

Fix type issue

cf13268

Fix type issue

94d129c

Reset migrations again

dcfb518

Restore originally generated migration

f34e0d4

Restore comment

e0ff8d7

jurajmajerik approved these changes Jan 30, 2025

View reviewed changes

Split migration into separate statements to avoid locking

b97a752

danielbachhuber commented Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(experiments): Tag support for shared metrics #28061

feat(experiments): Tag support for shared metrics #28061

danielbachhuber commented Jan 29, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jan 29, 2025

greptile-apps bot Jan 29, 2025

github-actions bot commented Jan 29, 2025 •

edited

Loading

raquelmsmith commented Jan 29, 2025 •

edited

Loading

danielbachhuber commented Jan 29, 2025

posthog-bot commented Jan 29, 2025

raquelmsmith commented Jan 29, 2025

posthog-bot commented Jan 29, 2025

posthog-bot commented Jan 29, 2025

posthog-bot commented Jan 30, 2025

posthog-bot commented Jan 30, 2025

jurajmajerik commented Jan 30, 2025

danielbachhuber commented Jan 30, 2025

jurajmajerik left a comment •

edited

Loading

danielbachhuber Jan 30, 2025

		@@ -0,0 +1,92 @@
		# Generated by Django 4.2.18 on 2025-01-30 17:45

feat(experiments): Tag support for shared metrics #28061

Are you sure you want to change the base?

feat(experiments): Tag support for shared metrics #28061

Conversation

danielbachhuber commented Jan 29, 2025 • edited Loading

Changes

How did you test this code?

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 29, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 29, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025 • edited Loading

raquelmsmith commented Jan 29, 2025 • edited Loading

danielbachhuber commented Jan 29, 2025

posthog-bot commented Jan 29, 2025

📸 UI snapshots have been updated

raquelmsmith commented Jan 29, 2025

posthog-bot commented Jan 29, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 29, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 30, 2025

📸 UI snapshots have been updated

posthog-bot commented Jan 30, 2025

📸 UI snapshots have been updated

jurajmajerik commented Jan 30, 2025

danielbachhuber commented Jan 30, 2025

jurajmajerik left a comment • edited Loading

Choose a reason for hiding this comment

danielbachhuber Jan 30, 2025

Choose a reason for hiding this comment

danielbachhuber commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

raquelmsmith commented Jan 29, 2025 •

edited

Loading

jurajmajerik left a comment •

edited

Loading