feat(coderd): add times_used to coder_apps in insights API
#13292
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| @@ -249,7 +249,7 @@ WITH | |||
| apps.slug, | |||
| apps.display_name, | |||
| apps.icon, | |||
| tus.app_usage_mins | |||
| (tus.app_usage_mins -> apps.slug)::smallint AS usage_mins | |||
There was a problem hiding this comment.
Drive-by optimization to avoid jsonb_each in the following CTE.
For now, only applied to `coder_app`s, same logic can be implemented for VS Code, SSH, etc. Part of #13099
8b606bf to
7347b81
Compare
| -- there can be duplicate start_times. To handle this, we use the | ||
| -- dense_rank() function, otherwise row_number() would suffice. | ||
| start_time - ( | ||
| dense_rank() OVER ( |
| times_used AS ( | ||
| SELECT DISTINCT ON (user_id, slug, display_name, icon, uniq) | ||
| slug, | ||
| display_name, |
There was a problem hiding this comment.
QQ: If somebody changes the display_name for the app, will it break the calculation? Probably a question related with other subqueries too.
There was a problem hiding this comment.
It'll handle it according to current behavior -- a change in display_name will be treated as a new app.
There was a problem hiding this comment.
But it should nit as technically the app is the same app.
There was a problem hiding this comment.
It depends, in aggregation across the whole deployment (which we don't show, but is possible), the apps might not be the same at all. For now I would not want to introduce one feature that behaves differently from the rest, we already show multiple entries for apps and parameters if something (like the display name) has changed.
There was a problem hiding this comment.
Yes. I am aware if the existing behavior and suggesting that we should change it sometimes in the future to be more robust to cosmetic changes to a coder_app.
There was a problem hiding this comment.
@matifali actually, have you recently confirmed this is still a problem? I think this is mainly a problem with parameters. If the app name changes it should only use the most recent name, at least since the rollup refactor.
| (SUM(ai.usage_mins) * 60)::bigint AS usage_seconds, | ||
| COALESCE(( | ||
| SELECT | ||
| COUNT(*) |
There was a problem hiding this comment.
It is just about cardinality of app usage, right?
There was a problem hiding this comment.
Yes, we are only interested in rows of this table, not of it's content.
| times_used AS ( | ||
| SELECT DISTINCT ON (user_id, slug, display_name, icon, uniq) | ||
| slug, | ||
| display_name, | ||
| icon, | ||
| -- Turn start_time into a unique identifier that identifies a users | ||
| -- continuous app usage. The value of uniq is otherwise garbage. | ||
| -- | ||
| -- Since we're aggregating per user app usage across templates, | ||
| -- there can be duplicate start_times. To handle this, we use the | ||
| -- dense_rank() function, otherwise row_number() would suffice. | ||
| start_time - ( | ||
| dense_rank() OVER ( | ||
| PARTITION BY | ||
| user_id, slug, display_name, icon | ||
| ORDER BY | ||
| start_time | ||
| ) * '30 minutes'::interval | ||
| ) AS uniq | ||
| FROM | ||
| template_usage_stats_with_apps |
There was a problem hiding this comment.
What sort of additional load does this place on the DB?
There was a problem hiding this comment.
I didn't measure but I don't expect it to be a lot, this is similar in weight (or lighter) as the app_insights CTE and could theoretically be run in parallel (don't know if PG will, though).
There was a problem hiding this comment.
I posted the explains, it seems to be parallel and very cheap.
Don't have a good dataset to test, but with a little bit of data it looks like this: |
|
Awesome! |
For now, only applied to
coder_apps, same logic can be implemented forVS Code, SSH, etc.
Part of #13099