Dashboard csv export v2 continued#6455
Conversation
|
| _ -> | ||
| %QueryResult{results: results} = QueryRunner.run(site, query) | ||
|
|
||
| Enum.map(results, fn r -> hd(r.dimensions) end) |
There was a problem hiding this comment.
issue, blocking: I think we should limit the amount of prop keys we query to prevent too many concurrent queries being run on CH in the next step.
There was a problem hiding this comment.
Yeah, makes sense. The query is currently unlimited but the queries are also executed synchronously. I've set a limit of 25 prop keys now in be7997c, which should be fine.
|
|
||
| defp get_custom_props_csv(_site, []), do: nil | ||
|
|
||
| defp get_custom_props_csv(site, prop_value_queries_by_prop_key) do |
There was a problem hiding this comment.
suggestion, non-blocking: This function has a lot of responsibilities so it's a bit hard to read. I think it could be split along these lines
- format header
- run queries
- format individual query result rows to array that matches header shape
- flatten output
- write to csv using a particular API
There was a problem hiding this comment.
I've split it into smaller functions here: 8f251f1
| }, | ||
| 'custom_props.csv': { | ||
| dimensions: ['event:props:*'], | ||
| metrics: getCustomPropsMetrics({ hasConversionGoalFilter: isGoalFilter }) |
There was a problem hiding this comment.
suggestion, non-blocking: The function getCustomPropsMetrics depends on isRevenueAvailable, which isn't passed.
It seems we actually want to say that in the CSV report, revenue is not available (isRevenueAvailable: false), or do we? With MetricContext allowing undefined, I can't actually tell what the intent is for this CSV report.
I think we shouldn't allow undefined in MetricContext.
Shorter args list at call sites is not worth losing explicitly declared intent at call sites.
There was a problem hiding this comment.
It seems we actually want to say that in the CSV report, revenue is not available (isRevenueAvailable: false), or do we? With MetricContext allowing undefined, I can't actually tell what the intent is for this CSV report.
Yeah, the intent is that revenue is not available. No specific reason other than sticking to legacy behaviour.
I think we shouldn't allow undefined in MetricContext.
IMHO it doesn't (currently) hurt readability because the behaviour between undefined and false is equivalent. Everything in the MetricContext that is irrelevant from the caller's perspective can simply be left unspecified and get interpreted as false, which, at least to me, is quite intuitive.
This would change with the addition of a context variable that would have a default value of true, but that's not the case, and I doubt it ever will be?
That said though, I've written out isRevenueAvailable: false to make this specific case explicit. 1632261.
2 participants
Changes
Add
custom_props.csvandconversions.csvto the v2 CSV export.Tests
Changelog
Documentation
Dark mode