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

Required filters: track when a filter is made required #39159

Merged
merged 1 commit into from Feb 27, 2024

Conversation

oleggromov
Copy link
Contributor

@oleggromov oleggromov commented Feb 26, 2024

Adds the dashboard_filter_required event and updates schemas. Closes #39066

What exactly is tracked

Every time the toggle is flipped to required, we will log an event.
Confirmed by Conor on Slack.

Testing

Tracking works locally with a setup described here.

@oleggromov oleggromov mentioned this pull request Feb 26, 2024
8 tasks
@oleggromov oleggromov added the no-backport Do not backport this PR to any branch label Feb 26, 2024
Copy link

replay-io bot commented Feb 26, 2024

Status In Progress ↗︎ 53 / 54
Commit 039bc0c
Results
⚠️ 4 Flaky
2319 Passed

@oleggromov oleggromov changed the title Required filters: track when a filter is made required WIP Required filters: track when a filter is made required Feb 26, 2024
@oleggromov oleggromov changed the title WIP Required filters: track when a filter is made required Required filters: track when a filter is made required Feb 26, 2024
@oleggromov oleggromov requested a review from a team February 26, 2024 16:45
@@ -298,6 +301,13 @@ export const setParameterRequired = createThunkAction(
required,
}));
}

if (required) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we track only when parameter becomes required from a not-required state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the case when required is true and paramater.required is also true?
We don't emit this action like that but I could add an additional check if you think it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually mean (if I understand the logic correctly) that we need to track when parameter wasn't required and then became required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I understood. In case we were emitting this action without checks (now because it's a "switch" you can't emit it when a parameter is already required), it would make sense. But broadly speaking, I don't think this is necessary.

This event is used to track feature adoption, which Conor agreed doesn't have to be very precise. That's why we didn't track it at saving for instance and instead track just toggling it on.

Copy link
Contributor

@uladzimirdev uladzimirdev left a comment

Choose a reason for hiding this comment

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

I think we need to update some test (probably e2e like it's done here) to be sure events are logged

@oleggromov oleggromov merged commit f7388f8 into master Feb 27, 2024
128 of 152 checks passed
@oleggromov oleggromov deleted the required-params-analytics branch February 27, 2024 10:17
Copy link

@oleggromov Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@oleggromov oleggromov added backport Automatically create PR on current release branch on merge and removed no-backport Do not backport this PR to any branch labels Feb 27, 2024
github-actions bot pushed a commit that referenced this pull request Feb 27, 2024
metabase-bot bot added a commit that referenced this pull request Feb 27, 2024
…#39195)

* Add dashboard_filter_required (#39159)

* Fix

---------

Co-authored-by: Oleg Gromov <mail@oleggromov.com>
@oleggromov oleggromov added this to the 0.49 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track required parameter toggling in analytics
2 participants