rusackas commented on PR #37516:
URL: https://github.com/apache/superset/pull/37516#issuecomment-3946522523
Running CI, and did a quick Claude Code review for good measure:
### Blocking concerns
#### 1. Security: Timing data always exposed, no way to disable
Per-phase timing is unconditionally returned in every /api/v1/chart/data
response. In multi-tenant deployments, this leaks operational details (cache
hit/miss status, DB execution time, validation overhead) to all authenticated
users. This should be behind a feature flag or at minimum a config toggle,
especially since Superset is used in security-sensitive environments.
#### 2. db_execution_ms inconsistency across layers
The three layers disagree on how to represent "no DB execution":
- TypeScript type: db_execution_ms: number | null (field present, value
null)
- Marshmallow schema: allow_none=True (field present, value null)
- Python code: key simply absent from the dict when cached
This will cause undefined vs null confusion on the frontend. The Python
code should set timing["db_execution_ms"] = None on cache hits to match the
schema/type contract.
#### 3. Stats logger units likely wrong
stats_logger.timing(f"chart_data.{phase}", value / 1000)
This divides milliseconds by 1000, sending seconds to
stats_logger.timing(). But BaseStatsLogger.timing() and statsd's timing metric
conventionally expect milliseconds. This would cause all metrics to appear
1000x smaller than actual.
--
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]
