Skip to content

fix(logs): increase json field for logs table #24911

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.

Sign up for GitHub

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

Jump to bottom
Merged
craig-rueda merged 2 commits into from
Aug 8, 2023
Merged

fix(logs): increase json field for logs table #24911

craig-rueda merged 2 commits into from
Aug 8, 2023

Conversation

@cnabro
Copy link
Contributor

@cnabro cnabro commented Aug 8, 2023 •
edited
Loading

SUMMARY

  • Increase json field for logs table. (Text to MediumText)
  • When insert logs while imports a specific dashboard, An error occured.
(MySQLdb._exceptions.DataError) (1406, "Data too long for column 'json' at row 1")
[SQL: INSERT INTO logs (`action`, user_id, dashboard_id, slice_id, json, dttm, duration_ms, referrer) VALUES (%s, %s, %s, %s, %s, %s, %s, %s)]
[parameters: ('copy_dash', '1', 9, 0, '{"path": "/superset/copy_dash/9/", "data": "{\\"certified_by\\":\\"\\",\\"certification_details\\":\\"\\",\\"css\\":\\"\\",\\"dashboard_title\\":\\"G ... (171666 characters truncated) ... BS\\"}},\\"filter_scopes\\":\\"{}\\"}", "url_rule": "/superset/copy_dash/<int:dashboard_id>/", "object_ref": "Superset.copy_dash", "dashboard_id": 9}', datetime.datetime(2023, 8, 7, 6, 50, 6, 897685), 6474, '[http://__/superset/dashboard/9/?native_filters_key=A27QtFLKT95Qdd0X-h4KkTLZTBhOslCg0Jshcisx4A8FM71hI_Ae6pBdTc4ac94f')](http://__/superset/dashboard/9/?native_filters_key=A27QtFLKT95Qdd0X-h4KkTLZTBhOslCg0Jshcisx4A8FM71hI_Ae6pBdTc4ac94f%27))]

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

0769ef90fddd -> 2e826adca42c (head), Fix schema for log
ee179a490af9 -> 0769ef90fddd, Fix schema perm for datasets
e0f6f91c2055 -> ee179a490af9, deckgl-path-width-units
bf646a0c1501 -> e0f6f91c2055, create_user_favorite_table
a23c6f8b1280 -> bf646a0c1501, json_metadata

TESTING INSTRUCTIONS

ALTER TABLE logs MODIFY json MEDIUMTEXT;
Query OK, 556821 rows affected (5.10 sec)
  • Estimate 100000 rows updated per 1 sec

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@cnabro cnabro requested a review from a team as a code owner August 8, 2023 05:27
@codecov
Copy link

codecov bot commented Aug 8, 2023 •
edited
Loading

Codecov Report

Merging #24911 (47a0eb0) into master (85a7d5c) will not change coverage.
The diff coverage is n/a.

❗ Current head 47a0eb0 differs from pull request most recent head fd3d4e5. Consider uploading reports for the commit fd3d4e5 to get more accurate results

@@           Coverage Diff           @@
##           master   #24911   +/-   ##
=======================================
  Coverage   69.00%   69.00%           
=======================================
  Files        1906     1906           
  Lines       74134    74134           
  Branches     8208     8208           
=======================================
  Hits        51153    51153           
  Misses      20858    20858           
  Partials     2123     2123
Flag Coverage Δ
hive 54.17% <ø> (ø)
mysql 79.21% <ø> (ø)
postgres 79.31% <ø> (ø)
presto 54.07% <ø> (ø)
python 83.37% <ø> (ø)
sqlite 77.89% <ø> (ø)
unit 55.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cnabro cnabro force-pushed the feature/log_json branch from 5f0e538 to fd3d4e5 Compare August 8, 2023 05:51
craig-rueda
craig-rueda approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM

@craig-rueda craig-rueda merged commit eb7c145 into apache:master Aug 8, 2023
@john-bodley
Copy link
Member

john-bodley commented Aug 8, 2023 •
edited
Loading

@cnabro thanks for the change. Would you mind also authoring a follow up PR which adds a line record to UPDATING.md which states that downtime is likely required in order to run this migration? A DDL operation (which locks the table) on the heavily used logs table will likely result in the service being inoperable.

Additionally I saw you checked the,

Runtime estimates and downtime expectations provided

checkbox, but I couldn't see any stats reported. Would you mind updating the PR description to include the necessary statistics.

@john-bodley john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Aug 8, 2023
@cnabro cnabro mentioned this pull request Aug 9, 2023
Merged
9 tasks
@cnabro
Copy link
Contributor Author

cnabro commented Aug 9, 2023 •
edited
Loading

@john-bodley Thank you. I'd added some notices in this PR (#24923).
And added additional descriptions for estimated times for this migration.

@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Aug 9, 2023
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 10, 2023
michael-s-molina pushed a commit that referenced this pull request Aug 10, 2023
@cnabro @michael-s-molina
fix(logs): increase json field for logs table (#24911)
ed56375 
(cherry picked from commit eb7c145)
@john-bodley
Copy link
Member

john-bodley commented Sep 27, 2023

@cnabro looking over your PR I noticed that you didn't actually update the SQLAlchemy model definition. Would you mind authoring a follow up PR to include the model change?

@cnabro
Copy link
Contributor Author

cnabro commented Sep 28, 2023

@john-bodley Ok. I'll give you another PR 😀

@cnabro cnabro mentioned this pull request Sep 28, 2023
Merged
9 tasks
@michael-s-molina michael-s-molina mentioned this pull request Feb 14, 2024
Merged
9 tasks
@mistercrunch mistercrunch added 🍒 3.0.0 Cherry-picked to 3.0.0 🍒 3.0.1 Cherry-picked to 3.0.1 🍒 3.0.2 Cherry-picked to 3.0.2 🍒 3.0.3 Cherry-picked to 3.0.3 🍒 3.0.4 Cherry-picked to 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 First shipped in 3.1.0 labels Mar 8, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@cnabro
fix(logs): increase json field for logs table (apache#24911)
46444eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@craig-rueda craig-rueda craig-rueda approved these changes

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 Cherry-picked to 3.0.0 🍒 3.0.1 Cherry-picked to 3.0.1 🍒 3.0.2 Cherry-picked to 3.0.2 🍒 3.0.3 Cherry-picked to 3.0.3 🍒 3.0.4 Cherry-picked to 3.0.4 🚢 3.1.0 First shipped in 3.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cnabro @john-bodley @craig-rueda @mistercrunch @michael-s-molina