fix(experiments): fix old aggregation_group_type_index being copied to the new query #20889
Conversation
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: 0 B 🆕 Total Size: 0 B |
| } as Node) | ||
| } as NodeWithSource | ||
|
|
||
| if (querySource['aggregation_group_type_index'] === undefined) { |
There was a problem hiding this comment.
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:
- Make sure
cleanFiltersreturns the undefined value (I think this wasn't reverted from the previous PR - we stopped passing in the aggregation group type index here) - Make sure
filtersToQueryNodealso 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:
- Takes the output of
filtersToQueryNodeand adds props we definitely want to be sent alongside. This isolates the change to our experiments code. - Or, add a non-empty configuration option to
filtersToQueryNodeso 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄.
There was a problem hiding this comment.
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)
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
Done in #21220 |
I've committed yesterday's solution just for reference.
Earlier discussion for reference: #20815 (comment)
Problem
After changing the "Participant type" from any group to Persons,
updateQuerySourcereceivesquerySourcewithoutaggregation_group_type_indexIt then creates a new query, however, it copies all the values from the existing
query.source—> this is where the oldaggregation_group_type_indexgets introduced.You suggested fixing this in:
cleanFilters- this will have no effect, asquerySourcereceived inupdateQuerySourceis already correctly cleaned up ofaggregation_group_type_indexqueryNodeToFilter- this won’t help either, because at this point, the old value has been added back to thequerySource, and because it’s not undefined, it will stay thereSo to sum up,
updateQuerySourceis exactly where the copying of the old value takes place. How else then should it be fixed? 🤔