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): define custom Trends exposure in the modal #27097

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jurajmajerik
Copy link
Contributor

@jurajmajerik jurajmajerik commented Dec 20, 2024

Problem

In the new metrics view, we no longer use the Goal component (see screenshot) to view and edit the custom exposure metric for Trends.

image

Changes

Add the ability to edit the exposure metric from within the Trends metric modal. This is for primary metrics only. Will repeat this for secondary metrics in a follow up.

image image

How did you test this code?

👀

@jurajmajerik jurajmajerik requested a review from a team December 20, 2024 11:05
Copy link
Contributor

github-actions bot commented Dec 20, 2024

Size Change: 0 B

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.11 MB

compressed-size-action

Copy link
Contributor

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Approving so you can land and iterate on Monday, assuming my questions have reasonable answers 😄

{
kind: NodeKind.EventsNode,
name: '$pageview',
event: '$pageview',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for using $pageview here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to set some default event here, and $pageview is our default everywhere. But maybe $feature_flag_called makes more sense.

trendsFilter: {
display: ChartDisplayType.ActionsLineGraph,
},
filterTestAccounts: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does dateRange and filterTestAccounts need to be hard-coded? It seems like they should be derived from other configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dateRange should be okay as is - it's suitable for the preview (last 14 days), and it gets updated in the query runner when calculating results.

filterTestAccounts - probably a good idea..

},
{
key: 'exposure',
label: 'Exposure',
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I didn't realize we accidentally dropped the exposure metric UI for Trends. I'm surprised we haven't gotten any tickets about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@jurajmajerik You're right. I was looking at a data warehouse trends experiment where we did remove the UI element.

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.

2 participants