chore: add 4xx error codes where applicable #21627

eschutho · 2022-09-28T18:59:12Z

Adding some 4xx error codes for user error, timeouts and alerts not run based on grace periods so that we can get more granular logging to differentiate between these different types of errors. Longer term fix, we probably don't need to raise and error for grace period fixes, perhaps, but instead a warning. I added a rate limit error, which seemed to be the closest that I could find. Would love any feedback on these choices.

TESTING INSTRUCTIONS

No user facing changes should be present. The logs should show 4xx errors for any user-generated reasons that an alert/report did not run.

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

codecov · 2022-09-28T19:35:02Z

Codecov Report

Merging #21627 (eae8656) into master (e2b77a7) will decrease coverage by 11.38%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #21627       +/-   ##
===========================================
- Coverage   66.66%   55.28%   -11.39%     
===========================================
  Files        1794     1794               
  Lines       68643    68677       +34     
  Branches     7301     7301               
===========================================
- Hits        45764    37970     -7794     
- Misses      21011    28839     +7828     
  Partials     1868     1868

Flag	Coverage Δ
hive	`52.93% <100.00%> (+0.03%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.83% <100.00%> (+0.03%)`	⬆️
python	`57.90% <100.00%> (-23.52%)`	⬇️
sqlite	`?`
unit	`50.94% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
superset/reports/commands/exceptions.py	`88.69% <100.00%> (-9.29%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/tags/core.py	`4.54% <0.00%> (-95.46%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-88.89%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-85.30%)`	⬇️
superset/key_value/commands/delete_expired.py	`0.00% <0.00%> (-80.77%)`	⬇️
superset/dashboards/commands/importers/v0.py	`15.62% <0.00%> (-76.25%)`	⬇️
superset/datasets/commands/update.py	`25.00% <0.00%> (-69.05%)`	⬇️
superset/datasets/commands/create.py	`31.25% <0.00%> (-68.75%)`	⬇️
superset/datasets/commands/importers/v0.py	`24.03% <0.00%> (-67.45%)`	⬇️
... and 285 more

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

hughhhh · 2022-09-28T22:52:11Z

superset/reports/commands/exceptions.py

@@ -163,10 +165,12 @@ class ReportScheduleExecuteUnexpectedError(CommandException):


 class ReportSchedulePreviousWorkingError(CommandException):
+    status = 429


nit: idk if this is a too many request fit here but i also don't what other code fits

Yeah, I agree. I wrote the same thing in the summary. I feel like ideally this wouldn't be an error.

I don't think this will ever generate an HTTP response, this is only used on execute.py that always run on the workers

Thanks @dpgaspar. The goal is to help log the user errors vs system errors more granularly. Right now they are all getting logged as one event. Do you think that the status codes is a viable approach for logging purposes?

error_type seems more appropriate to me, similar to:

superset/superset/exceptions.py

Line 118 in 383313b

class SupersetSyntaxErrorException(SupersetErrorsException):

Thanks @dpgaspar! @john-bodley, @betodealmeida and I were talking about error_types as well and concluded that errors should not be raised, only exceptions, and that we can include errors with an exception, but not raise them. Do you have thoughts on that @dpgaspar?

eschutho · 2022-09-29T00:23:34Z

marking this as a draft as I need to look into how this change affects the customer facing logs.

dpgaspar

execute.py is always used on a worker context

dpgaspar · 2022-09-29T08:51:49Z

superset/reports/commands/exceptions.py

@@ -163,10 +165,12 @@ class ReportScheduleExecuteUnexpectedError(CommandException):


 class ReportSchedulePreviousWorkingError(CommandException):
+    status = 429


I don't think this will ever generate an HTTP response, this is only used on execute.py that always run on the workers

dpgaspar · 2022-09-29T08:52:25Z

superset/reports/commands/exceptions.py

    message = _("Report Schedule is still working, refusing to re-compute.")


 class ReportScheduleWorkingTimeoutError(CommandException):
+    status = 408


same here, please recheck

pkdotson · 2022-10-03T20:46:04Z

superset/reports/commands/execute.py

@@ -526,7 +527,7 @@ def next(self) -> None:
                    return
            self.send()
            self.update_report_schedule_and_log(ReportState.SUCCESS)
-        except CommandException as first_ex:
+        except Exception as first_ex:


Are you changing these to more general exceptions because the CommandException does not return the full trace?

rather because all of the exceptions here may not inherit from CommandException since I'm raising them all individually.

pkdotson

LGTM!

sadpandajoe · 2022-10-03T23:22:38Z

🏷️ preset:2022.39

(cherry picked from commit 4417c6e)

pull-request-size bot added the size/S label Sep 28, 2022

eschutho requested review from dpgaspar and betodealmeida September 28, 2022 18:59

eschutho force-pushed the elizabeth/fix-alert-errors branch from eae8656 to 4eaa6d2 Compare September 28, 2022 20:17

pull-request-size bot added size/M and removed size/S labels Sep 28, 2022

eschutho changed the title ~~add 4xx error codes where applicable~~ chore: add 4xx error codes where applicable Sep 28, 2022

hughhhh reviewed Sep 28, 2022

View reviewed changes

eschutho marked this pull request as draft September 29, 2022 00:23

dpgaspar reviewed Sep 29, 2022

View reviewed changes

eschutho force-pushed the elizabeth/fix-alert-errors branch from 4eaa6d2 to 3b0ceff Compare September 29, 2022 16:27

eschutho marked this pull request as ready for review September 29, 2022 16:50

add 4xx error codes where applicable

df4e6f7

eschutho force-pushed the elizabeth/fix-alert-errors branch from 3b0ceff to df4e6f7 Compare October 3, 2022 19:50

pkdotson reviewed Oct 3, 2022

View reviewed changes

pkdotson approved these changes Oct 3, 2022

View reviewed changes

eschutho merged commit 4417c6e into apache:master Oct 3, 2022

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Oct 3, 2022

chore: add 4xx error codes where applicable (apache#21627)

777e22f

(cherry picked from commit 4417c6e)

eschutho deleted the elizabeth/fix-alert-errors branch October 21, 2022 20:59

eschutho mentioned this pull request Nov 8, 2022

refactor: use exception status for logging #21971

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add 4xx error codes where applicable #21627

chore: add 4xx error codes where applicable #21627

eschutho commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

hughhhh Sep 28, 2022

eschutho Sep 28, 2022 •

edited

Loading

dpgaspar Sep 29, 2022

eschutho Sep 29, 2022

dpgaspar Sep 30, 2022

eschutho Oct 28, 2022

eschutho commented Sep 29, 2022

dpgaspar left a comment

dpgaspar Sep 29, 2022

dpgaspar Sep 29, 2022

pkdotson Oct 3, 2022 •

edited

Loading

eschutho Oct 3, 2022

pkdotson left a comment

sadpandajoe commented Oct 3, 2022

		@@ -163,10 +165,12 @@ class ReportScheduleExecuteUnexpectedError(CommandException):


		class ReportSchedulePreviousWorkingError(CommandException):
		status = 429

chore: add 4xx error codes where applicable #21627

chore: add 4xx error codes where applicable #21627

Conversation

eschutho commented Sep 28, 2022 • edited Loading

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

eschutho Sep 28, 2022 • 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

eschutho commented Sep 29, 2022

dpgaspar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdotson Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdotson left a comment

Choose a reason for hiding this comment

sadpandajoe commented Oct 3, 2022

eschutho commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

eschutho Sep 28, 2022 •

edited

Loading

pkdotson Oct 3, 2022 •

edited

Loading