-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Create an analytics_summary
database view for analytics
#2450
Conversation
🤖 Hasura Change Summary compared a subset of table metadata including permissions: Tracked Tables (1) |
t.slug as team_slug, | ||
a.type as analytics_type, | ||
a.created_at as analytics_created_at, | ||
a.user_agent as user_agent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping we can keep this as a JSONB column with the changes in #2451
If not, I can just pull out the key bits of data here like browser, platform etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with initially keeping as JSONB!
nit: a.user_agent as user_agent
is redundant / unnecessary and adds a bit of noise? only need as
if actually renaming, table reference won't be visible in final view (applies to a number of columns below too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
Removed vultr server and associated DNS entries |
analytics_summary
database view for analytics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'll let Mike do final approval of this one as he's most familiar with pain points around current joins 👍
t.slug as team_slug, | ||
a.type as analytics_type, | ||
a.created_at as analytics_created_at, | ||
a.user_agent as user_agent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with initially keeping as JSONB!
nit: a.user_agent as user_agent
is redundant / unnecessary and adds a bit of noise? only need as
if actually renaming, table reference won't be visible in final view (applies to a number of columns below too)
978b838
to
e251c1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 🥳
I really think this will make building questions via the Metabase GUI way easier and make the extra features associated with that available!
One thing I suppose we should maybe keep in mind going forward is that any new columns like allowlist
which are created would benefit from being added to this view.
Thanks for final review, Mike - I'll merge this ✔️ Are you happy to take care of making visible in Metabase and updating SQL so we can eventually "hide" raw tables if no longer necessary? (as you go, doesn't need to be an immediate swap!) |
Once we've side stepped the name change issue I'd be happy to pick this up 👍 |
What does this PR do?
submission_services_summary
database view for analytics #2430JOIN
operations in the GUI we were making Silvia do redundant 🤞