Skip to content
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

fix: Operator dash user counts, performance #839

Merged
merged 4 commits into from
May 31, 2024

Conversation

bmtcril
Copy link
Contributor

@bmtcril bmtcril commented May 29, 2024

Don't use the user_pii table for user counts, since it may not be populated. Change this chart to be a count of active enrollments, which people have said is more useful.

Also changed the time range for operator dash to 90 days to improve performance of the actors / events graphs.

Closes: #788

@bmtcril bmtcril requested review from Ian2012 and saraburns1 May 29, 2024 15:32
bmtcril added 2 commits May 29, 2024 16:34
Don't use the user_pii table for user counts, since it may not be populated. Change this chart to be a count of active enrollments, which people have said is more useful.

Also changed the time range for operator dash to 90 days to improve performance of the actors / events graphs.
@bmtcril bmtcril force-pushed the bmtcril/operator_dash_fixes branch from 17a05e8 to 0cc17dc Compare May 30, 2024 14:07
-- Need to cast the course key to a string here otherwise the
-- course_names dictionary throws this:
-- Key type for complex key at position 0 does not match, expected String, found LowCardinality(String).
on course_names.course_key = enrollment.course_key::String
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this affect DBT reports too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dbt recreates everything with the type of the parent, so they almost all get converted to LowCardinality. LC types get automatically coerced into their parent type (String for us) in almost all cases, there have only been two places I found where it gets confused, both around dictionary joins.

Copy link
Contributor

Choose a reason for hiding this comment

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

And custom reports that do the same join such as the fact_*_engagement ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact_*_engagement charts are working as-is, but I can't guarantee what other custom reports might run into. That's likely a problem with a number of changes we've been making, though.

tutoraspects/plugin.py Outdated Show resolved Hide resolved
@Ian2012 Ian2012 merged commit 508383e into main May 31, 2024
9 checks passed
@Ian2012 Ian2012 deleted the bmtcril/operator_dash_fixes branch May 31, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator dash shows incorrect user information with PII off
2 participants