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.

Sign up for GitHub

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): fix old aggregation_group_type_index being copied to the new query #20889

Closed
wants to merge 3 commits into from
Closed

fix(experiments): fix old aggregation_group_type_index being copied to the new query #20889

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e61f962
add yesterda's fix
jurajmajerik Mar 13, 2024
f9de96e
Update UI snapshots for `chromium` (2)
github-actions[bot] Mar 13, 2024
b4d8bda
Update UI snapshots for `chromium` (2)
github-actions[bot] Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions frontend/src/scenes/insights/insightVizDataLogic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,13 +348,24 @@ export const insightVizDataLogic = kea<insightVizDataLogicType>([
actions.updateInsightFilter({ display })
},
updateQuerySource: ({ querySource }) => {
actions.setQuery({
interface NodeWithSource {
kind: NodeKind
source: QuerySourceUpdate
}

const newQuery = {
...values.query,
source: {
...values.querySource,
...handleQuerySourceUpdateSideEffects(querySource, values.querySource as InsightQueryNode),
},
} as Node)
} as NodeWithSource

if (querySource['aggregation_group_type_index'] === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, the problem with this fix is that it breaks what updateQuerySource is supposed to do: i.e only do additive updates. I'm not 100% sure where the fix should go, but definitely sure this isn't the right way.

Consider how there's no problem with this in regular insights, this just introduces an exception that shouldn't be there. For example, now, whenever we call updateQuerySource without the aggregation_group_type_index, it resets it to persons aggregation, which is not ok.

The problem here that needs to be fixed is in our code: Our querySource never returns an undefined for aggregation_group_type_index, and instead omits it, because there are several 'gates' it needs to pass through:

  1. Make sure cleanFilters returns the undefined value (I think this wasn't reverted from the previous PR - we stopped passing in the aggregation group type index here)
  2. Make sure filtersToQueryNode also returns the group_type_index, which is currently removed when undefined.

Now, it doesn't make sense for me to update filtersToQueryNode - it's doing the right thing it's supposed to do, discard every random empty thing and create a clean Query node. Our problem is that experiments want to use the output of this for additive updates, where the group type index being undefined needs to pass through.

Thus, I suggest creating a helper function for experiments that:

  1. Takes the output of filtersToQueryNode and adds props we definitely want to be sent alongside. This isolates the change to our experiments code.
  2. Or, add a non-empty configuration option to filtersToQueryNode so it returns all undefined things as well.

Currently prefer (1), but filtersToQueryNode seem to be majorly used by experiments only. @thmsobrmlr since you wrote this, any ideas / is there a different filter to query function we should be using for updates? (or @mariusandra )

The problematic bits in question start here: https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/experiments/experimentLogic.tsx#L386

Copy link
Contributor

Choose a reason for hiding this comment

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

Without looking deeper (1) sounds like a good suggestion. I'll try to have a look at this later as well.

is there a different filter to query function we should be using for updates?

No, there is only the filtersToQueryNode function.

Also note that we're actively working on removing all filters and the filtersToQueryNode and cleanFilters functions with it. If you're doing a bigger refactoring, transitioning all the way to queries is the way to go.

Copy link
Collaborator

@mariusandra mariusandra Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, not using filters, but query directly is the way to go. All insights use query natively for example, and we're deprecating this old style.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm on the backend we're still storing filters for experiments, so not yet ready to completely get rid of them, but I'm guessing what you're suggesting is that while we're in client-side land, all interactions ought to happen using the query, and then at the end when saving we can convert it into a filter so the backend can continue using it to calculate metrics?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah exactly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All HogQL insights use "query" in the backend natively, and once we're done with the refactor and ready to drop legacy insights, the "filters" object will no longer be used anywhere... except for the experiments backend then 😄.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Experiments backend passes on the filter to Trends / Funnel query runner, we don't do any manual custom queries, so I guess once we're ready to drop legacy insights we should easily migrate too :D - let me know when you're about ready, I'm assuming you'll have some way to convert saved insight filters to query, we'll need a similar mechanism for experiments.

(Juraj - your call to make for the frontend query changes, my suggestion would be to do this Query migration right after we've done the UI revamp (vs. now), unless in the middle of the revamp you feel things become much easier to do the query migration as well)

delete newQuery.source['aggregation_group_type_index']
}

actions.setQuery(newQuery)
},
setQuery: ({ query }) => {
if (isInsightVizNode(query)) {
Expand Down
Loading