Session time series #659
Session time series #659
Conversation
|
Noting #668 because it came about after working on this PR. Adding the line graph was pretty trivial but there's growing number of magic strings to navigate which made it more effort than needed. We could benefit from refactoring these out. |
| @@ -16,7 +15,7 @@ export function ActionsLineGraph({ dashboardItemId = null, filters: filtersParam | |||
| }, [toParams(otherFilters)]) | |||
|
|
|||
| return results ? ( | |||
| results.reduce((total, item) => total + item.count, 0) > 0 ? ( | |||
| filters.session || results.reduce((total, item) => total + item.count, 0) > 0 ? ( | |||
There was a problem hiding this comment.
Referring to comment above. I opted to use a bunch of spot if/elses like this to reduce possible collision with existing logic for this feature. #668 should go back and better centralize how a bunch of conditions like this throughout trends are working
| @@ -112,7 +112,10 @@ export class LineGraph extends Component { | |||
| ) | |||
| return null | |||
| var label = data.datasets[tooltipItem.datasetIndex].label || '' | |||
| return label + ' - ' + tooltipItem.yLabel.toLocaleString() | |||
| let formattedLabel = label + ' - ' + tooltipItem.yLabel.toLocaleString() | |||
| if (formattedLabel.includes('Average Duration of Session')) | |||
There was a problem hiding this comment.
This feels super brittle. Maybe this should just be a prop that you pass a function to (labelFormat or something), then we can pass that from ActionsLineGraph where at least we have access to the filter object
There was a problem hiding this comment.
I changed it so that the backend passes an extra chartLabel attribute in case we want to different labels for charts and tables. Let me know fi this works
| @@ -185,7 +188,7 @@ export class LineGraph extends Component { | |||
| } | |||
| } | |||
| LineGraph.propTypes = { | |||
| datasets: PropTypes.arrayOf(PropTypes.shape({ label: PropTypes.string, count: PropTypes.number })).isRequired, | |||
| datasets: PropTypes.arrayOf(PropTypes.shape({ label: PropTypes.string, count: PropTypes.any })).isRequired, | |||
There was a problem hiding this comment.
Restructured the labels so this is number again!
| overall_average = {'label': 'Average Duration of Session', 'count': avg_formatted} | ||
|
|
||
| cursor = connection.cursor() | ||
| cursor.execute(average_length_time(all_sessions), sessions_sql_params) |
There was a problem hiding this comment.
These queries aren't cheap. I wonder if you could combine them into one query, maybe by getting both the average and the number of sessions for each day and then summing that.
Also, in overall_average_length, the query might be a bit quicker if you do SELECT COUNT(1) instead of COUNT(*).
There was a problem hiding this comment.
Condensed this so that it calculates the average outside the query and only has to use one query to get the time series data and totals. This could be a different slow down but I think calculating an average serverside rather than submitting a whole query for that one number is better. Though, this hasn't been clocked so I'm not completely sure.
There was a problem hiding this comment.
It's still a bit slow (50s with a few million events) though no more so than before
|
Sorry, merge conflict! Happy to merge after :) Edit: Having had a last play (and sorry for reversal on this) but I think it would be good to convert seconds into human readable in the frontend. It's fine with lower numbers, but can anyone intuitively understand what '1400 seconds' means? We could push to different PR but for the experience we should do this. |
|
Yeah, good point. Made an issue here #680 so we can discuss how to display when there are an assortment of different values |
Changes
Todo:
Checklist