Skip to content

feat: expose insights into user activity #9807

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.

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

Merged
merged 26 commits into from
Sep 26, 2023
Merged

feat: expose insights into user activity #9807

merged 26 commits into from
Sep 26, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Sep 21, 2023

Related: #9497

This PR extends Insights API with the user activity. BTW I moved golden files for TestTemplateInsights_Golden to a dedicated subdirectory.

Note: I know that this PR is large, mainly due to generated golden files. I'm happy to split the PR into pieces if it helps!

TODO:

  • model structures in codersdk
  • coderd logic
  • database query
  • dbfake logic
  • unit tests

template_id,
start_time,
seconds
FROM combined_stats
Copy link
Member

@mafredri mafredri Sep 22, 2023

Choose a reason for hiding this comment

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

You could remove this additional step if you change UNION ALL to UNION above, it'll remove duplicates.

However, in this instance I'm not sure if we should sum template IDs separately. Say you use template A and B concurrently for 1h, you will be listed as 2h in the stats now. Perhaps instead we should modify this distinct to be a group by (user, start time) with array_agg on the templates. Not sure about this, but putting it out there...

Copy link
Member Author

Choose a reason for hiding this comment

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

You could remove this additional step if you change UNION ALL to UNION above, it'll remove duplicates.

Done! Thanks for the suggestion.

Say you use template A and B concurrently for 1h, you will be listed as 2h in the stats now.

It might be a business question, as it indicates the total duration the user spent working with the specific template. I would rather keep it as is rather than "online presence". Anyway, I will describe and cover it with tests 👍

@mtojek mtojek marked this pull request as ready for review September 26, 2023 08:59
@mtojek
Copy link
Member Author

mtojek commented Sep 26, 2023

@mafredri Let's start the next review round.

I implemented the entire workflow and covered responses with golden files. My biggest concern is TestUserActivityInsights_Golden function which is a fork/ripoff of the other large function. Do you think that I should extract common structures/functions? I added a few changes:

  • disabled/removed parameters
  • use dbgen.* to create users/membership/tokens

@mtojek mtojek requested a review from mafredri September 26, 2023 11:35
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Looks good! Don't worry about refactoring to remove duplication for now. We can defer it until there's 1-2 more use-cases or when we start having trouble keeping the data in sync.

"user_id": "00000000-0000-0000-0000-000000000004",
"username": "user1",
"avatar_url": "",
"seconds": 32400
Copy link
Member

Choose a reason for hiding this comment

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

Note-to-self: At first I thought this looked off, comparing the template golden file for the same setup, but upon verifying with the data it looks good, there is indeed 9h of distinct usage. 👍🏻

"user_id": "00000000-0000-0000-0000-000000000004",
"username": "user1",
"avatar_url": "",
"seconds": 30540
Copy link
Member

Choose a reason for hiding this comment

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

Note-to-self: Also verified this number, 1h is lost from users[0].workspaces[1] due to timeframe and all the minute intervals are summed from users[0].workspaces[0] giving us 8h + 29m. The hours in u0w0 overlap with u0w1 so we don't see an increase from those.

@mtojek mtojek merged commit 4c3b579 into main Sep 26, 2023
@mtojek mtojek deleted the 9497-db-first branch September 26, 2023 16:42
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants