-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(query_log): Hackathon access metrics from query_log in PostHog #24263
Conversation
…nto feat/query-log-metrics
EU_INSTANCE_TEAM_ID = 1 | ||
US_INSTANCE_TEAM_ID = 2 |
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.
Should these be constants or rather settings (thinking of self hosting folks)?
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.
Or better still, injected via env vars from charts
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.
Im fine with this, but best to get @mariusandra to double check some of the team id guarding stuff before merging please
EU_INSTANCE_TEAM_ID = 1 | ||
US_INSTANCE_TEAM_ID = 2 |
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.
Or better still, injected via env vars from charts
or (get_instance_region() == "US" and team_id == US_INSTANCE_TEAM_ID) | ||
or settings.DEBUG | ||
): | ||
database = InternalDatabase(timezone=team.timezone, week_start_day=team.week_start_day) |
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.
Not sure if InternalDatabase
is the correct naming here (nor whether subclassing Database
. I was imagining you using setattr(..)
to add the new tables to an existing Database
object
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.
Issue with setattr
was that these are class variables so if we set them once, then they'll persist on the class. Separate classes solves this issue
Going to hold off on this and try to get query_log in as a data warehouse table as when we start persisting it, it will flow through S3. So doesn't make sense to override the team id logic/add more complexity, if it'll just be a temporary solution :( |
Problem
Hackathon - Improve monitoring
Don't have query metrics in PostHog
Changes
Expose query_log as a lazy table so we can use it to create insights with monitoring metrics.
Does this work well for both Cloud and self-hosted?
N/A
How did you test this code?