-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: Add dashboard widget snapshot model #95236
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: Add dashboard widget snapshot model #95236
Conversation
This PR has a migration; here is the generated SQL for for --
-- Create model DashboardWidgetSnapshot
--
CREATE TABLE "sentry_dashboardwidgetsnapshot" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "data" text NOT NULL, "widget_id" bigint NOT NULL);
ALTER TABLE "sentry_dashboardwidgetsnapshot" ADD CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da" FOREIGN KEY ("widget_id") REFERENCES "sentry_dashboardwidget" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardwidgetsnapshot" VALIDATE CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da";
CREATE INDEX CONCURRENTLY "sentry_dashboardwidgetsnapshot_widget_id_8a0347ce" ON "sentry_dashboardwidgetsnapshot" ("widget_id"); |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95236 +/- ##
==========================================
- Coverage 87.73% 85.53% -2.20%
==========================================
Files 10548 10520 -28
Lines 607769 606756 -1013
Branches 23817 23675 -142
==========================================
- Hits 533215 519016 -14199
- Misses 74258 87382 +13124
- Partials 296 358 +62 |
This PR has a migration; here is the generated SQL for for --
-- Create model DashboardWidgetSnapshot
--
CREATE TABLE "sentry_dashboardwidgetsnapshot" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "data" text NOT NULL, "widget_id" bigint NOT NULL);
ALTER TABLE "sentry_dashboardwidgetsnapshot" ADD CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da" FOREIGN KEY ("widget_id") REFERENCES "sentry_dashboardwidget" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardwidgetsnapshot" VALIDATE CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da";
CREATE INDEX CONCURRENTLY "sentry_dashboardwidgetsnapshot_widget_id_8a0347ce" ON "sentry_dashboardwidgetsnapshot" ("widget_id"); |
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.
👍 Shruthi and I talked previously about this storing the JSON data used to reconstruct a widget, and we could use the serializer to update the widgets if we need to backtrack. The model is aligned with that plan
This PR has a migration; here is the generated SQL for for --
-- Create model DashboardWidgetSnapshot
--
CREATE TABLE "sentry_dashboardwidgetsnapshot" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "data" text NOT NULL, "widget_id" bigint NOT NULL);
ALTER TABLE "sentry_dashboardwidgetsnapshot" ADD CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da" FOREIGN KEY ("widget_id") REFERENCES "sentry_dashboardwidget" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardwidgetsnapshot" VALIDATE CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da";
CREATE INDEX CONCURRENTLY "sentry_dashboardwidgetsnapshot_widget_id_8a0347ce" ON "sentry_dashboardwidgetsnapshot" ("widget_id"); |
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.
Database changes lgtm.
One other option here could be to store the data in a temporary column on the existing table, but both methods seem about the same to me. Is the idea that you'll write a backup of the data when you're performing the migration?
This PR has a migration; here is the generated SQL for for --
-- Create model DashboardWidgetSnapshot
--
CREATE TABLE "sentry_dashboardwidgetsnapshot" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "data" text NOT NULL, "widget_id" bigint NOT NULL);
ALTER TABLE "sentry_dashboardwidgetsnapshot" ADD CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da" FOREIGN KEY ("widget_id") REFERENCES "sentry_dashboardwidget" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_dashboardwidgetsnapshot" VALIDATE CONSTRAINT "sentry_dashboardwidg_widget_id_8a0347ce_fk_sentry_da";
CREATE INDEX CONCURRENTLY "sentry_dashboardwidgetsnapshot_widget_id_8a0347ce" ON "sentry_dashboardwidgetsnapshot" ("widget_id"); |
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.
Bug: Model Meta and Field Default Mismatches
The DashboardWidgetSnapshot
model, introduced in the diff, has two issues:
- It is missing a
Meta
class withapp_label="sentry"
anddb_table
definitions, which is inconsistent with other models in the same file (e.g.,DashboardWidget
). This can lead to inconsistent table naming and ORM behavior. - The
data
JSONField's model definition omitsdefault=dict
, despite the corresponding migration specifying it. This inconsistency can cause the Django ORM to require the field or behave unexpectedly when creating instances, as it won't recognize the database-level default.
src/sentry/models/dashboard_widget.py#L260-L268
sentry/src/sentry/models/dashboard_widget.py
Lines 260 to 268 in 4fc728a
@region_silo_model | |
class DashboardWidgetSnapshot(Model): | |
__relocation_scope__ = RelocationScope.Organization | |
widget = FlexibleForeignKey("sentry.DashboardWidget") | |
data: models.Field[dict[str, Any], dict[str, Any]] = JSONField() | |
Was this report helpful? Give feedback by reacting with 👍 or 👎
yup, that's right! |
We'd like to store a snapshot of the last valid transaction widget while we do an in-place migration of widgets from transactions -> spans. This allows us to recover the original transaction state of the widget in case something goes horribly wrong.
We'd like to store a snapshot of the last valid transaction widget
while we do an in-place migration of widgets from transactions -> spans.
This allows us to recover the original transaction state of the
widget in case something goes horribly wrong.