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

fix(experiments): Standardize trends cont. exposure query logic #27647

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

Conversation

danielbachhuber
Copy link
Contributor

@danielbachhuber danielbachhuber commented Jan 17, 2025

See #26713

Changes

Standardizes trends continuous exposure query logic to use $feature_flag_called or the custom exposure event, not a count of the count event.

Also ensures filterTestAccounts is applied to default trend exposure queries.

How did you test this code?

Tests should pass. This branch hasn't actually been hit since #27432, though.

@danielbachhuber danielbachhuber requested a review from a team January 17, 2025 12:28
Copy link
Contributor

@andehen andehen left a comment

Choose a reason for hiding this comment

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

Looks good! 🧹

Tested it locally with two different trend metrics.

Yeah, after the mentioned PR the removed branch should no longer have been visited (at least by users in prod). I haven't see this bug since then either.

@danielbachhuber
Copy link
Contributor Author

Yeah, after the mentioned PR the removed branch should no longer have been visited (at least by users in prod).

Actually, fwiw, I think the branch was still visited. Your PR modifies prepared_count_query but the exposure query uses the original self.query.count_query.

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