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

feat(perf-detector-threshold-configuration) Added datamigration for c… #53109

Merged
merged 6 commits into from
Jul 24, 2023

Conversation

Abdkhan14
Copy link
Contributor

@Abdkhan14 Abdkhan14 commented Jul 18, 2023

For issue: #51315
Steps outlined: here.

  • The option to add widgets with world map visualization has been removed from the frontend: PR
  • Won't be merging this PR until the changes above are merged.
  • Now we are left with existing World map widgets.
Screenshot 2023-07-19 at 10 54 55 AM
  • This data migration intends to convert the worldmap widgets to table widgets with the following changes:
    • Display type of Widget is changed from 5 (world map) to 4 (table).
    • 'has:geo_country_code' condition is addded to each Widget Query.
    • Added 'geo.country_code' and 'geo.region' columns to each Widget Query.
    • Added 'geo.country_code' and 'geo.region' fields to each Widget Query.
    • Aggregates and order columns remain unchanged.

…onverting worldmap widgets to table widgets.
@Abdkhan14 Abdkhan14 requested a review from a team July 18, 2023 20:27
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 18, 2023
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0516_migrate_worldmap_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0516_migrate_world_map_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #53109 (8970130) into master (d3093d7) will decrease coverage by 3.46%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #53109      +/-   ##
==========================================
- Coverage   79.54%   76.09%   -3.46%     
==========================================
  Files        4941     4939       -2     
  Lines      208722   208670      -52     
  Branches    35551    35551              
==========================================
- Hits       166028   158784    -7244     
- Misses      37632    44770    +7138     
- Partials     5062     5116      +54     

see 485 files with indirect coverage changes

if "geo.region" not in widgetQuery.columns:
widgetQuery.columns.insert(0, "geo.region")
if "geo.country_code" not in widgetQuery.columns:
widgetQuery.columns.insert(0, "geo.country_code")
Copy link
Member

Choose a reason for hiding this comment

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

Why do we insert to the start of the list here? Is the order important?

Copy link
Contributor Author

@Abdkhan14 Abdkhan14 Jul 23, 2023

Choose a reason for hiding this comment

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

Yeah we care about the order here. We want country_code and region to be the first two columns/fields. @wedamija

@Abdkhan14 Abdkhan14 requested a review from wedamija July 23, 2023 14:24
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0519_migrate_worldmap_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

Copy link
Contributor

@mjq-sentry mjq-sentry left a comment

Choose a reason for hiding this comment

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

🗺️

@Abdkhan14 Abdkhan14 enabled auto-merge (squash) July 24, 2023 19:05
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0520_migrate_world_map_widgets.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

@Abdkhan14 Abdkhan14 merged commit 824e856 into master Jul 24, 2023
55 checks passed
@Abdkhan14 Abdkhan14 deleted the abdkhan/dnd-worldmap-widget-migration branch July 24, 2023 19:32
@markstory markstory added the Trigger: Revert add to a merged PR to revert it (skips CI) label Jul 24, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 6543790

getsentry-bot added a commit that referenced this pull request Jul 24, 2023
…on for c… (#53109)"

This reverts commit 824e856.

Co-authored-by: markstory <24086+markstory@users.noreply.github.com>
Abdkhan14 added a commit that referenced this pull request Jul 25, 2023
…53475)

- Migration already approved here:
[PR](#53109) but it had a
runtime error.
- Added null checks for columns and fields in DashboardWidgetQuery.
- Should fix run time error from previous migration attempt: 
<img width="488" alt="Screenshot 2023-07-24 at 6 17 48 PM"
src="https://github.com/getsentry/sentry/assets/60121741/49fe3214-77c3-4bdb-b02f-355eb567f1d3">

- Link to failure:
https://deploy.getsentry.net/go/tab/build/detail/deploy-getsentry-backend-us/63/migrations/1/migrations
- Tested it out locally with NULL columns.

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
chloeho7 pushed a commit that referenced this pull request Jul 25, 2023
…53475)

- Migration already approved here:
[PR](#53109) but it had a
runtime error.
- Added null checks for columns and fields in DashboardWidgetQuery.
- Should fix run time error from previous migration attempt: 
<img width="488" alt="Screenshot 2023-07-24 at 6 17 48 PM"
src="https://github.com/getsentry/sentry/assets/60121741/49fe3214-77c3-4bdb-b02f-355eb567f1d3">

- Link to failure:
https://deploy.getsentry.net/go/tab/build/detail/deploy-getsentry-backend-us/63/migrations/1/migrations
- Tested it out locally with NULL columns.

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
Abdkhan14 added a commit that referenced this pull request Jul 25, 2023
…ver chart. (#53185)

For issue: #51315
Steps outlined:
[here](https://www.notion.so/sentry/Removing-World-Map-display-from-Discover-and-Dashboards-e49eb198fe294ad0a1b4c47b9c3619c0?pvs=4#43e2c318d1eb4f93987718b82307f56f).

- Won't be merging until, data-migration
[PR](#53109) for converting
world map widgets to table widgets is deployed.
- Removes the World map display mode from discover chart footer:
<img width="1104" alt="Screenshot 2023-07-19 at 3 59 54 PM"
src="https://github.com/getsentry/sentry/assets/60121741/80ca9e8e-aeb4-45ab-92a4-bda951b426f5">

- Should not effect the loading of existing discover saved queries,
homepages and previews with world map display mode:
- - Falls back to AreaChart instead of WorldMapChart:
[code](https://github.com/getsentry/sentry/blob/4bd6934f1b3f947043ef93b1dd35a11b1e9927cf/static/app/components/charts/eventsChart.tsx#L140-L163)
- - Falls back to Total Period display mode instead:
[code](https://github.com/getsentry/sentry/blob/9b816eae27f4a29eedad50e5ed8af86188badec1/static/app/utils/discover/eventView.tsx#L1403-L1426)
- Removed tests and blocks of code using: `DisplayMode.WORLDMAP`.
- Added unused components like `WorldMapChart` and `EventsGeoRequest` to
clean up list for removal.

---------

Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants