feat(alerts & reports): Easier to read execution logs #13752

jfrag1 · 2021-03-23T20:37:32Z

SUMMARY

Currently, reading execution logs for alerts and reports can be confusing. Several log entries can be created by a single execution run, but nothing indicates that they are part of the same execution, except that they have the same timestamp.

This PR gives each execution a UUID which is truncated to 6 characters on the execution log view. This way, it will be much easier to tell which log entries belong to which execution.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

I've only tested manually.

ADDITIONAL INFORMATION

codecov · 2021-03-23T21:17:08Z

Codecov Report

Merging #13752 (fac8f58) into master (467848d) will increase coverage by 1.99%.
The diff coverage is 83.33%.

❗ Current head fac8f58 differs from pull request most recent head 039f056. Consider uploading reports for the commit 039f056 to get more accurate results

@@            Coverage Diff             @@
##           master   #13752      +/-   ##
==========================================
+ Coverage   75.56%   77.56%   +1.99%     
==========================================
  Files         935      935              
  Lines       47275    47286      +11     
  Branches     5883     5883              
==========================================
+ Hits        35725    36679     +954     
+ Misses      11375    10463     -912     
+ Partials      175      144      -31

Flag	Coverage Δ
cypress	`56.07% <ø> (+8.37%)`	⬆️
hive	`80.29% <77.77%> (+<0.01%)`	⬆️
javascript	`63.75% <100.00%> (+<0.01%)`	⬆️
mysql	`80.60% <77.77%> (+<0.01%)`	⬆️
postgres	`80.63% <77.77%> (+<0.01%)`	⬆️
presto	`80.30% <77.77%> (+<0.01%)`	⬆️
python	`81.14% <77.77%> (+<0.01%)`	⬆️
sqlite	`80.19% <77.77%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/views/CRUD/alert/types.ts	`100.00% <ø> (ø)`
superset/reports/logs/api.py	`95.45% <ø> (ø)`
superset/tasks/scheduler.py	`0.00% <0.00%> (ø)`
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx	`100.00% <100.00%> (ø)`
superset/models/reports.py	`100.00% <100.00%> (ø)`
superset/reports/commands/execute.py	`93.58% <100.00%> (+0.14%)`	⬆️
...erset-frontend/src/SqlLab/components/ResultSet.tsx	`68.64% <0.00%> (-0.30%)`	⬇️
...set-frontend/src/dashboard/util/getDropPosition.js	`90.90% <0.00%> (ø)`
superset-frontend/src/components/ListView/utils.ts	`85.95% <0.00%> (+0.82%)`	⬆️
.../src/explore/components/ControlPanelsContainer.tsx	`97.27% <0.00%> (+0.90%)`	⬆️
... and 113 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f7845...039f056. Read the comment docs.

ktmud · 2021-03-23T22:28:13Z

Why do we have duplicate execution IDs in the screenshot? Is it because of testing?

ktmud · 2021-03-23T22:31:03Z

superset/migrations/versions/301362411006_add_execution_id_to_report_execution_.py

+def upgrade():
+    op.add_column(
+        'report_execution_log',
+        sa.Column('execution_id', sa.VARCHAR(length=50), nullable=True)


Can we simply call it uuid and use sa.Column('uuid', UUIDType(binary=True)) just to be consistent with other entities in Superset?

superset/superset/migrations/versions/c501b7c653a3_add_missing_uuid_column.py

Line 69 in b34c863

sa.Column(

Yes! I wasn't aware there was a column type specifically for UUID's.

Since we'll be having multiple log entries with the same UUID, would it be confusing to name the column uuid?

I think we can call it UUID internally and just change the display name.

nytai · 2021-03-23T22:33:40Z

@ktmud The screenshot is of the execution log, so multiple entries for each alert/report execution is expected. The id is tied to a single execution/run, for which there will be multiple log entries. The id makes it easier to visually group the logs for each run.

@jfrag1 Do we have designs for this? Wondering if adding the "Execution ID" after "State" makes more sense

jfrag1 · 2021-03-23T22:38:52Z

@nytai No, I'm not aware of any designs. It would be good to get some designer eyes on this.

ktmud · 2021-03-23T22:58:57Z

What does the log page as a whole look like?

I think a simple way to improve the visual grouping is to maybe set rowspan on the "Execution ID" column (and any other common columns). If all log entries are guaranteed to have the same info for almost all columns, then maybe you can just show one row per execution and have multiple box/links for different log entry in one "Logs" cell.

jfrag1 · 2021-03-23T23:10:07Z

Here is what the entire row looks like:

I believe the only columns guaranteed to be the same are execution id and value. The timestamps are the same currently, but there are plans to change them to show milliseconds so that order of execution be be more clearly seen, so those will be different.

nytai · 2021-03-23T23:13:55Z

I think it's also possible for runs to overlap. Eg, if one execution is taking a while (slow query, capturing screenshot) and the next execution begins.

mihir174 · 2021-03-23T23:26:47Z

Hey, just catching up with context here! Is this PR referring to the log that appears after clicking the Log button in the actions column of an alert or report?

I worked on designs for alerts & reports a while ago. There weren't any designs produced for the log at the time since the team wasn't sure columns would need to be included. If this is what i think it is, some thoughts -

the ID to group / differentiate between execution events makes sense
the State column should live on the far left

jfrag1 · 2021-03-23T23:30:35Z

@mihir174 Yes, this is the log shown after clicking the Log button. I'll switch the State and Execution ID columns. Thanks for the input!

github-actions · 2021-03-24T18:29:24Z

⚠️ @jfrag1 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

dpgaspar

Looking good, left a couple of comments. We should also update tests and make sure we get these exact columns there, here: https://github.com/apache/superset/blob/master/tests/reports/api_tests.py#L1074

dpgaspar · 2021-03-26T13:23:33Z

superset/models/reports.py

@@ -171,6 +172,7 @@ class ReportExecutionLog(Model):  # pylint: disable=too-few-public-methods

    __tablename__ = "report_execution_log"
    id = Column(Integer, primary_key=True)
+    uuid = Column(UUIDType(binary=True))


We probably should think about moving out of sqlalchemy_utils...

We're already pretty tied to it - this doesn't tie us any deeper. I think it's OK for now.

dpgaspar · 2021-03-26T13:26:26Z

superset/reports/commands/execute.py

@@ -403,6 +407,7 @@ def __init__(
        self._session = session
        self._report_schedule = report_schedule
        self._scheduled_dttm = scheduled_dttm
+        self._execution_id = uuid4()


Have you checked if we can use celery's UUID, and pass it into the AsyncExecuteReportScheduleCommand constructor?

The closest thing I found to this is this Github issue, but it only described how it could be done when triggering tasks with delay or apply_async, and I wasn't sure how to do it for a task triggered by a beat schedule.

Actually scratch that, I tweaked my Google search and found this, let me see if I can get it to work.

The delay is invoked by the caller.

use <job_name>.request.id to get the id, then use it here:
https://github.com/apache/superset/blob/master/superset/tasks/scheduler.py#L67

on this case should be execute.request.id

check if you need to use the bind=True and then self.request.id to bind the function to the task class.
https://docs.celeryproject.org/en/latest/userguide/tasks.html?highlight=requestcontext#basics

Yep, execute.request.id worked!

jfrag1 · 2021-03-26T16:52:54Z

Here's what the list view looks like now with the state column at the far left:

willbarrett

LGTM

willbarrett · 2021-03-29T20:34:56Z

@jfrag1 I think this needs a rebase.

jfrag1 · 2021-03-29T20:36:38Z

On it!

dpgaspar

LGTM

* master: (56 commits) test: Adds tests and storybook to CertifiedIcon component (#13457) chore: Moves CheckboxIcons to Checkbox folder (#13459) chore: Removes Popover duplication (#13462) build(deps): bump elliptic from 6.5.3 to 6.5.4 in /docs (#13527) fix: allow spaces in DB names (#13800) chore: Update PR template for SIP-59 DB migrations process (#13855) Add CODEOWNERS (#13759) feat(alerts & reports): Easier to read execution logs (#13752) fix: Disallows negative options remaining (#13749) Fix broken link (#13861) fix(native-filters): add global async query support to native filters (#13837) Displays row limit warning with Alert component (#13854) fix(errors): Downgrade error on stop query to a warning (#13826) fix(alerts and reports): Unify timestamp format on execution log view (#13718) fix(sqllab): warning message when rows limited (#13841) chore: add success log whenever a connection is working (#13811) fix(native-filters): improve loading styles for filter component (#13794) chore: update change log with cherry-picks for release 1.1 (#13824) feat: added support to configure the default explorer viz (#13610) fix(#13734): Properly escape special characters in CSV output (#13735) ...

* Prep for migration * Migration for execution id column * Generate execution ids for alerts and reports * Change execution id range * Add execution id to API endpoint * Add execution id to execution log view * Change execution id range * Change execution id to a uuid * Fix execution id type * Switch state and exec. id columns * Change db column to UUIDType * Python lint * Fix failing frontend tests * execution_id -> uuid * Fix migration head * lint * Use celery task id as the execution id * lint * lint for real * Fix tests

betodealmeida · 2021-07-20T00:09:35Z

superset/migrations/versions/301362411006_add_execution_id_to_report_execution_.py

+
+
+def downgrade():
+    op.drop_column("report_execution_log", "execution_id")


The column execution_id doesn't exist.

pull-request-size bot added the size/M label Mar 23, 2021

jfrag1 changed the title ~~Easier to read execution logs~~ feat(alerts & reports): Easier to read execution logs Mar 23, 2021

lilykuang added the risk:db-migration PRs that require a DB migration label Mar 23, 2021

ktmud reviewed Mar 23, 2021

View reviewed changes

jfrag1 force-pushed the easier-to-read-execution-logs branch 2 times, most recently from fb7283e to 2bdc232 Compare March 26, 2021 03:09

dpgaspar reviewed Mar 26, 2021

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Mar 28, 2021

willbarrett approved these changes Mar 29, 2021

View reviewed changes

jfrag1 added 7 commits March 29, 2021 13:39

Prep for migration

16d86a9

Migration for execution id column

a81244c

Generate execution ids for alerts and reports

63bb69e

Change execution id range

e4c00f6

Add execution id to API endpoint

6fdb6e2

Add execution id to execution log view

48d3922

Change execution id range

7e6ad79

jfrag1 added 13 commits March 29, 2021 13:39

Change execution id to a uuid

c88c46e

Fix execution id type

4c2db0b

Switch state and exec. id columns

a36a7bf

Change db column to UUIDType

e61edb0

Python lint

e28eb47

Fix failing frontend tests

5f9f492

execution_id -> uuid

5bb43ff

Fix migration head

64e5cdc

lint

078d5ee

Use celery task id as the execution id

32e9ce7

lint

bd8bc66

lint for real

461411f

Fix tests

039f056

jfrag1 force-pushed the easier-to-read-execution-logs branch from aa48d1a to 039f056 Compare March 29, 2021 20:39

dpgaspar approved these changes Mar 30, 2021

View reviewed changes

willbarrett merged commit 304e3b3 into apache:master Mar 30, 2021

jfrag1 deleted the easier-to-read-execution-logs branch March 30, 2021 15:58

jfrag1 mentioned this pull request Apr 7, 2021

fix: execution log crashes for logs with no uuid #13998

Merged

betodealmeida reviewed Jul 20, 2021

View reviewed changes

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alerts & reports): Easier to read execution logs #13752

feat(alerts & reports): Easier to read execution logs #13752

jfrag1 commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

ktmud commented Mar 23, 2021

ktmud Mar 23, 2021 •

edited

Loading

jfrag1 Mar 23, 2021

jfrag1 Mar 23, 2021

nytai Mar 23, 2021

nytai commented Mar 23, 2021 •

edited

Loading

jfrag1 commented Mar 23, 2021

ktmud commented Mar 23, 2021

jfrag1 commented Mar 23, 2021

nytai commented Mar 23, 2021

mihir174 commented Mar 23, 2021

jfrag1 commented Mar 23, 2021 •

edited

Loading

github-actions bot commented Mar 24, 2021

dpgaspar left a comment

dpgaspar Mar 26, 2021 •

edited

Loading

willbarrett Mar 29, 2021

dpgaspar Mar 26, 2021

jfrag1 Mar 26, 2021

jfrag1 Mar 26, 2021

dpgaspar Mar 26, 2021

dpgaspar Mar 26, 2021

jfrag1 Mar 26, 2021

jfrag1 commented Mar 26, 2021 •

edited

Loading

willbarrett left a comment

willbarrett commented Mar 29, 2021

jfrag1 commented Mar 29, 2021

dpgaspar left a comment

betodealmeida Jul 20, 2021



		def downgrade():
		op.drop_column("report_execution_log", "execution_id")

feat(alerts & reports): Easier to read execution logs #13752

feat(alerts & reports): Easier to read execution logs #13752

Conversation

jfrag1 commented Mar 23, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

ktmud commented Mar 23, 2021

ktmud Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nytai commented Mar 23, 2021 • edited Loading

jfrag1 commented Mar 23, 2021

ktmud commented Mar 23, 2021

jfrag1 commented Mar 23, 2021

nytai commented Mar 23, 2021

mihir174 commented Mar 23, 2021

jfrag1 commented Mar 23, 2021 • edited Loading

github-actions bot commented Mar 24, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrag1 commented Mar 26, 2021 • edited Loading

willbarrett left a comment

Choose a reason for hiding this comment

willbarrett commented Mar 29, 2021

jfrag1 commented Mar 29, 2021

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrag1 commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

ktmud Mar 23, 2021 •

edited

Loading

nytai commented Mar 23, 2021 •

edited

Loading

jfrag1 commented Mar 23, 2021 •

edited

Loading

dpgaspar Mar 26, 2021 •

edited

Loading

jfrag1 commented Mar 26, 2021 •

edited

Loading