rebenitez1802 commented on code in PR #45862:
URL: https://github.com/apache/superset/pull/45862#discussion_r4419062079
##########
superset/models/core.py:
##########
@@ -103,6 +103,13 @@
is_oauth2_retry_active,
OAuth2ClientConfigSchema,
)
+from superset.utils.timing_metrics import (
+ elapsed_ms,
+ emit_timing,
+ metric_suffix,
+ now_ns,
+ timed,
+)
Review Comment:
Goes with the `database.connect` suggestion below, which leaves
`elapsed_ms`, `emit_timing` and `now_ns` unused:
```suggestion
from superset.utils.timing_metrics import metric_suffix, timed
```
##########
superset/charts/data/api.py:
##########
@@ -575,7 +591,10 @@ def _process_data(query_data: Any) -> Any:
if dashboard_filter_context is not None:
payload["dashboard_filters"] =
dashboard_filter_context.to_dict()
- with
event_logger.log_context(f"{self.__class__.__name__}.json_dumps"):
+ with (
+ timed("chart_data.response.serialize"),
+
event_logger.log_context(f"{self.__class__.__name__}.json_dumps"),
Review Comment:
**🟡 Medium — `chart_data.response.serialize` includes the event-log DB
write**
`timed()` is the outer context manager here, so it also covers the exit of
`event_logger.log_context`, which with the default `DBEventLogger` is a
synchronous INSERT + COMMIT to the metadata DB. With a 50 ms metadata write the
timer reads ~52–55 ms while `json.dumps` takes ~0.03 ms (`database.execute`
already nests correctly inside `log_context("execute_sql")`). Swapping the
order fixes it:
```suggestion
event_logger.log_context(f"{self.__class__.__name__}.json_dumps"),
timed("chart_data.response.serialize"),
```
##########
superset/models/core.py:
##########
@@ -1032,7 +1039,12 @@ def _log_query(sql_: str) -> None:
security_manager,
)
+ engine_suffix = metric_suffix(self.db_engine_spec.engine)
+ connect_start_ns = now_ns()
with self.get_raw_connection(catalog=catalog, schema=schema) as conn:
+ emit_timing(
+ f"database.connect.{engine_suffix}",
elapsed_ms(connect_start_ns)
+ )
Review Comment:
**🟡 Medium — `database.connect.<engine>` is never emitted when connecting
fails**
The timer is emitted inside the `with` body, so a failed or timed-out
connect emits nothing: a 2 s postgres connect timeout produced no
`database.connect.postgresql` while `chart_data.query.data_acquisition` read
2032 ms, contrary to "slow failures such as timeouts stay visible". Timing only
the context manager's entry via `ExitStack` (same pattern as
`_open_with_oauth2_retry`) fixes it; OAuth2 handling for connect and body
errors is unchanged, and the rest of the block keeps its indentation. Apply
together with the import suggestion at the top of the file.
```suggestion
engine_suffix = metric_suffix(self.db_engine_spec.engine)
with ExitStack() as stack:
with timed(f"database.connect.{engine_suffix}"):
conn = stack.enter_context(
self.get_raw_connection(catalog=catalog, schema=schema)
)
```
--
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]
