rusackas commented on code in PR #40585:
URL: https://github.com/apache/superset/pull/40585#discussion_r3337032444


##########
superset/sql_lab.py:
##########
@@ -121,7 +122,10 @@ def handle_query_error(
         query.set_extra_json_key("errors", errors_payload)
 
     db.session.commit()
+    logger.exception("Query %s failed: %s", query.id, ex)

Review Comment:
   Fixed in commit 8b8b0c12e1. Removed the `logger.exception` call from inside 
`handle_query_error` in `superset/sql_lab.py`. The caller at line 199 already 
calls `logger.exception("Query %d: %s", query_id, ex)`, so the inner log was 
producing a duplicate traceback. Only the caller-side logging is retained.



##########
superset/sql/execution/celery_task.py:
##########
@@ -99,9 +100,12 @@ def _handle_query_error(
         query.set_extra_json_key("errors", errors_payload)
 
     db.session.commit()  # pylint: disable=consider-using-transaction
+    logger.exception("Query %s failed: %s", query.id, ex)

Review Comment:
   Fixed in commit 8b8b0c12e1. Removed the `logger.exception` call from inside 
`_handle_query_error` in `superset/sql/execution/celery_task.py`. Its sole 
caller at line 354 already calls `logger.exception("Query %d: %s", query_id, 
ex)`, so the inner call was a duplicate. Caller-side logging is retained.



##########
superset/sql_lab.py:
##########
@@ -121,7 +122,10 @@ def handle_query_error(
         query.set_extra_json_key("errors", errors_payload)
 
     db.session.commit()
+    logger.exception("Query %s failed: %s", query.id, ex)
     payload.update({"status": query.status, "error": msg, "errors": 
errors_payload})
+    if stacktrace := traceback.format_exc():
+        payload["stacktrace"] = stacktrace

Review Comment:
   Fixed in commit 8b8b0c12e1. `handle_query_error` in `superset/sql_lab.py` 
now gates the `stacktrace` field on `app.config.get("SHOW_STACKTRACE")`. Since 
`SHOW_STACKTRACE = False` is the default in `superset/config.py` (line 328), 
raw Python tracebacks are not included in the payload for unprivileged users. 
Operators/admins can enable it by setting `SHOW_STACKTRACE = True` in their 
config.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to