-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore: improve analytics #11714
chore: improve analytics #11714
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11714 +/- ##
==========================================
- Coverage 65.67% 63.56% -2.12%
==========================================
Files 905 449 -456
Lines 43862 27663 -16199
Branches 4201 0 -4201
==========================================
- Hits 28806 17583 -11223
+ Misses 14915 10080 -4835
+ Partials 141 0 -141
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@dpgaspar, looking for ways to cover things like |
d4bad5b
to
927de84
Compare
927de84
to
c6018f5
Compare
@dpgaspar this is ready for review! |
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, left some comments and the duplicate needs to be removed
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, left one last comment
9d53e7d
to
aab4d2d
Compare
@dpgaspar , I think I addressed all the comments |
Hmm, sorry for being late to the party. But I think |
@mistercrunch, @dpgaspar, et al. would you be able to provide more as what the new fields added to the Also did you consider using the cc: @bkyryliuk (unsure if Dropbox uses MySQL). |
@robdiciuccio or @willbarrett do either you (or anyone at Preset) know the motivations for these changes? Wrangling this migration has already cost Airbnb quite some time and our current work around (to prevent significant downtime) is a costly and timely operation. Prior to rolling out this migration to production we just want to be sure that this change is really needed and permanent in nature, i.e., we would hate to have to perform a similar migration in the near future to rollback this logic. |
@john-bodley looking into it and will respond shortly |
Clearly this PR has 2 main parts:
It sounds like db migration on |
@ktmud I agree about the About reverting (if that's on the table), we kind of need to alter the upgrade to be upgrade script to be noop AND add a downgrade script that would remove the columns if they exist as people are already on this migration. |
@mistercrunch we have ~190 million events in the table currently, and while we may have a solution to migrate it, I think we've historically tried to avoid major changes to the logs table since migrations can be quite costly. The last migration before yours that touched the table was in 2018, and it already includes the If we do think the approach i mentioned here makes sense (putting your new fields in the json column). Then perhaps we can fix forward with a new migration, and include details on how to skip past the 2 migrations adding and removing the new columns in UPDATING.md |
In general I sense we should not be storing columns (or fields in JSON objects, etc.) which can easily be derived from other columns, i.e., As I mentioned in this comment and as @etr2460 also stated using the |
@mistercrunch I think for things as heavy as that requiring a db migration, a little bit more instrumentation is warranted. I'd vote for revert before this goes out to more users. My two cents on the value of the added columns:
I agree both |
SUMMARY
Adding a few fields to what's logged:
/superset/explore_json/
/api/v1/chart/<int>/
ChartRestApi.post
Also instrumenting more events in
/superset/views/core.py
, and the dashboard and chart APIs