fix(api): include query lifecycle timing in /api/v1/chart/data response#37516
fix(api): include query lifecycle timing in /api/v1/chart/data response#37516YuriyKrasilnikov wants to merge 47 commits into
Conversation
Code Review Agent Run #5007cdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
api
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Fixed in b8d12f31.
|
Code Review Agent Run #a58248Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #f0aa4aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Added in bd6a3d6TypeScript interface for
|
Code Review Agent Run #d72779Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #37516 +/- ##
==========================================
- Coverage 65.13% 65.12% -0.01%
==========================================
Files 2753 2755 +2
Lines 154573 155534 +961
Branches 35453 35572 +119
==========================================
+ Hits 100674 101296 +622
- Misses 51985 52269 +284
- Partials 1914 1969 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CI Failures Analysis1. pre-commit (current/next/previous)yarn registry returned 500 Internal Server Error — infrastructure issue, unrelated to this PR. 2. sharded-jest-tests (7)Jest worker crash on 3. Python Integration Tests (test-sqlite, test-mysql, test-postgres)Problem: Root cause: References:
Options:
Which option is preferred? |
Fixed in eba466aOpenAPI spec validation fixProblem: Root cause analysis: The issue was NOT in # Before (broken)
timing = fields.Nested(
ChartDataTimingSchema,
metadata={...},
allow_none=True,
load_default=None, # ← causes "default": null in OpenAPI
)When Testing results:
Fix: Removed # After (works)
timing = fields.Nested(
ChartDataTimingSchema,
metadata={...},
allow_none=True,
) |
Code Review Agent Run #d5da15Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #97f5c2Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Running CI, and did a quick Claude Code review for good measure: Blocking concerns1. Security: Timing data always exposed, no way to disablePer-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 layersThe three layers disagree on how to represent "no DB execution":
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 wrongstats_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. |
|
Thanks for the review @rusackas. 1. Security — timing always exposed 2. 3. |
Code Review Agent Run #ec367fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Fixes apache#37507 Add per-phase timing breakdown to get_df_payload() using time.perf_counter(). The timing dict includes validate_ms, cache_lookup_ms, db_execution_ms (cache miss only), result_processing_ms, total_ms, and is_cached flag. Each phase is also emitted to STATS_LOGGER.timing(). New config CHART_DATA_SLOW_QUERY_THRESHOLD_MS enables WARNING logging when total_ms exceeds the threshold.
Code Review Agent Run #75cdceActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Pushed two follow-up fixes after reviewing the latest CodeAnt The first fix separates a contribution total's internal dependency from its The updated contract is:
The second fix handles a partially resolved native annotation request explicitly. Regression coverage now includes exact reuse in both materialized and cache-only Verification:
The repository-wide Ruff/Pylint hooks still report only the unrelated existing |
Code Review Agent Run #d7b5f6Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Reviewed the additional CodeAnt comments and addressed the The contribution cache-identity finding was valid, but I did not add The unhashable extra-cache-key finding was also valid, although it was not a new The streaming validation finding was correct as well. The command intentionally Regression coverage verifies producer-position stability, producer-identity Verification:
|
Code Review Agent Run #2c1bcfActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Query-specific cache validation could discard a loaded legacy entry after the cache manager had already accepted a cache-only request. Forced refresh paths could likewise return an unloaded cache object, allowing datasource execution even though the caller required cached data. - Validate filter metadata before any datasource acquisition. - Reject unusable legacy entries and forced refreshes when cache-only execution is requested. - Preserve stale-entry refresh behavior for requests that permit source execution. Regression coverage requires both legacy filtered entries and explicitly forced refreshes to fail without contacting the datasource.
The drill-detail filter regression test still targeted a private helper removed by the typed acquisition refactor, causing the master merge to fail during collection instead of exercising the maintained execution path. - Exercise DRILL_DETAIL through acquire_query_data. - Assert the prepared query is isolated from the caller and retains filters. - Verify the prepared query reaches the cache-aware acquisition boundary. The characterization boundary remains limited to backend preparation and does not claim frontend filter propagation.
Several values introduced along the timing, cache, SQL planning, and streaming export paths relied on inference even though project policy requires explicit types for new Python variables. The implicit declarations also obscured contracts at shared execution boundaries. - Declare timing limits and cached dataframe state. - Type query materialization and streaming export locals. - Use the concrete SqlaQuery result type for deferred SQL planning. Static analysis requires these declarations to remain compatible with MyPy, Ruff, and the project Pylint plugins.
Code Review Agent Run #0c7c9fActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Upstream time-comparison changes overlapped query-context processor coverage after contribution totals moved from in-place mutation to dependency-aware acquisition. Retaining the obsolete totals test would restore a contract that no longer exists, while taking only the branch side would discard new time-offset guarantees. - Retain quarter, zero-shift, and invalid-offset coverage with the corresponding parser and alignment behavior.\n- Preserve immutable contribution planning and typed chart-data timing coverage.\n- Incorporate the remaining upstream fixes without altering their behavior.
| @contextmanager | ||
| def chart_timing_phase(phase: ChartTimingPhase) -> Iterator[None]: | ||
| """Measure one fixed chart API phase when request collection is active.""" | ||
| timing = _request_timing.get() |
There was a problem hiding this comment.
Suggestion: Add an explicit local type annotation for this request timing value to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new Python code introduces a local variable that is inferable but not explicitly annotated. Under the type-hint rule, this is a relevant variable that can be annotated, so the suggestion identifies a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/timing.py
**Line:** 70:70
**Comment:**
*Custom Rule: Add an explicit local type annotation for this request timing value to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| def _emit_request_metrics(timing: ChartRequestTiming, total_ns: int) -> None: | ||
| try: | ||
| stats_logger = current_app.config.get("STATS_LOGGER") |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation for this logger-like local variable so the new code includes type hints for relevant variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is newly added Python code and the local variable is used as a logger-like object but has no explicit type annotation. That fits the type-hint requirement for relevant variables, so the suggestion is valid.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/timing.py
**Line:** 130:130
**Comment:**
*Custom Rule: Add a concrete type annotation for this logger-like local variable so the new code includes type hints for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| for query_result in execution.queries: | ||
| error = query_result.payload.get("error") | ||
| status = query_result.payload.get("status") |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the newly introduced local variables in the execution-results loop to satisfy the type-hint requirement for relevant variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The modified Python code introduces two local variables, error and status, without type annotations even though their types can be inferred and annotated. This matches the type-hint rule for relevant variables in new or changed code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/chart/warm_up_cache.py
**Line:** 109:111
**Comment:**
*Custom Rule: Add explicit type annotations for the newly introduced local variables in the execution-results loop to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| # Each offset must start from the original request state. | ||
| query_object_clone = copy.copy(query_object) |
There was a problem hiding this comment.
Suggestion: Add an explicit variable type annotation for this cloned query object to satisfy the type-hint requirement for newly introduced variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is a newly added local variable in modified Python code and it can be annotated with QueryObject; the rule requires type hints for relevant variables that can be annotated, so the omission is a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1900:1901
**Comment:**
*Custom Rule: Add an explicit variable type annotation for this cloned query object to satisfy the type-hint requirement for newly introduced variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| with source_timing(SourceKind.TIME_OFFSET, SourceProvider.SQL): | ||
| result = self.query(query_object_clone_dct) |
There was a problem hiding this comment.
Suggestion: Annotate this new local result variable with its expected return type to comply with the rule requiring type hints in modified code. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is a newly introduced local variable in modified Python code, and it can be annotated as QueryResult; the code currently omits that type hint, matching the rule violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2122:2123
**Comment:**
*Custom Rule: Annotate this new local result variable with its expected return type to comply with the rule requiring type hints in modified code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if defer_source_queries: | ||
| prequery = self.get_query_str_extended( | ||
| prequery_obj, | ||
| mutate=False, | ||
| defer_source_queries=True, | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add a type annotation to this newly introduced local variable so its expected structure is explicit and type-checkable. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This branch introduces a new local variable whose return type is known (QueryStringExtended), so it should be annotated under the type-hint rule; the existing code omits that annotation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 4263:4268
**Comment:**
*Custom Rule: Add a type annotation to this newly introduced local variable so its expected structure is explicit and type-checkable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| class SourceTimingCollector: | ||
| """Collect a bounded source tree without retaining query content.""" | ||
|
|
||
| def __init__(self, clock: Callable[[], int] = time.perf_counter_ns) -> None: |
There was a problem hiding this comment.
Suggestion: Add an explicit instance attribute type annotation for this assigned callable so static type checking can validate collector clock usage. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The attribute is assigned in init without an explicit instance annotation even though its type is known from the constructor signature. This matches the custom rule requiring type hints for relevant annotatable variables in new Python code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/chart_data_timing.py
**Line:** 386:386
**Comment:**
*Custom Rule: Add an explicit instance attribute type annotation for this assigned callable so static type checking can validate collector clock usage.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ) -> tuple[QueryAcquisitionResult, str | None, int]: | ||
| """Acquire AUTO currency metadata through the normal cache-aware path.""" | ||
| datasource = _get_datasource(query_context, query_obj) | ||
| form_data = query_context.form_data or {} |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this new local variable so static type checking can validate accesses to form data fields. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new local variable form_data is assigned a dictionary-like value but has no explicit type annotation. This matches the custom rule requiring type hints on relevant variables that can be annotated, so the violation is real.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_actions.py
**Line:** 156:156
**Comment:**
*Custom Rule: Add an explicit type annotation for this new local variable so static type checking can validate accesses to form data fields.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| from_dttm=query_obj.from_dttm, | ||
| to_dttm=query_obj.to_dttm, | ||
| extras=query_obj.extras, | ||
| currency_query = copy.copy(query_obj) |
There was a problem hiding this comment.
Suggestion: Add a concrete type hint for this copied query object variable to keep the new query-preparation flow fully typed. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The copied query object is a newly introduced local variable and is used as a QueryObject, but it is not explicitly annotated. This is a valid instance of the type-hint rule violation described in the custom rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/query_actions.py
**Line:** 179:179
**Comment:**
*Custom Rule: Add a concrete type hint for this copied query object variable to keep the new query-preparation flow fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| with chart_timing_phase("authorize"): | ||
| command.validate() |
There was a problem hiding this comment.
Suggestion: The new authorization step in the cache endpoint calls validate() but does not handle QueryObjectValidationError, so malformed/invalid cached query contexts will now bubble up as 500s instead of a client error response. Add a QueryObjectValidationError handler (consistent with the other chart data endpoints) around this validation path and return a 400 response. [api mismatch]
Severity Level: Major ⚠️
- ❌ Async chart results endpoint errors on invalid cached queries.
- ⚠️ Clients see 500 instead of actionable validation message.Steps of Reproduction ✅
1. Enable global async queries so that chart async jobs use the Celery task
`load_chart_data_into_cache` at `superset/tasks/async_queries.py:8-40`, which on success
stores a query context in the cache and returns a URL `/api/v1/chart/data/<cache_key>`
(lines 20-36).
2. In a controlled test or development environment, construct a cached query context entry
whose `form_data` produces a `QueryObject` with invalid configuration, for example
duplicate metric/column labels that cause `QueryObject.validate()` to raise
`QueryObjectValidationError` as implemented in `superset/common/query_object.py:85-110`
(duplicate labels path at lines 100-110), and store it under a cache key that
`QueryContextCacheLoader.load()` will return (loader defined in
`superset/charts/data/query_context_cache_loader.py:23-30`).
3. Issue an HTTP GET request to `/api/v1/chart/data/<cache_key>`, which is handled by
`ChartDataRestApi.data_from_cache` in `superset/charts/data/api.py:135-199`; inside this
method, the try-block at lines 181-187 loads the cached form data via
`_load_query_context_form_from_cache`, builds a `QueryContext` with
`_create_query_context_from_form`, and constructs `command =
ChartDataCommand(query_context)`.
4. Still inside `data_from_cache`, the new authorization phase at lines 188-189 calls
`command.validate()` within `with chart_timing_phase("authorize")`, which routes to
`QueryContext.raise_for_access()` (`superset/common/query_context.py:52-53`) and then
`QueryContextProcessor.raise_for_access()`
(`superset/common/query_context_processor.py:25-40`) that iterates queries and calls
`query.validate()`, causing `QueryObject.validate()` to raise `QueryObjectValidationError`
for the invalid query; this exception is not caught by the surrounding `except` clauses in
`data_from_cache` (which only handle `ChartDataCacheLoadError`,
`SupersetSecurityException`, and `ValidationError` at
`superset/charts/data/api.py:190-197), so it propagates and results in a 500 Internal
Server Error instead of the 400 response used by the other chart data endpoints
(`get_data` and `data` both catch `QueryObjectValidationError` at
`superset/charts/data/api.py:241-252` and 90-101 respectively).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 447:448
**Comment:**
*Api Mismatch: The new authorization step in the cache endpoint calls `validate()` but does not handle `QueryObjectValidationError`, so malformed/invalid cached query contexts will now bubble up as 500s instead of a client error response. Add a `QueryObjectValidationError` handler (consistent with the other chart data endpoints) around this validation path and return a 400 response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| def get_extra_cache_keys(self, query_obj: QueryObjectDict) -> list[Any]: | ||
| return [] |
There was a problem hiding this comment.
Suggestion: Returning an unconditional empty list here makes cache keys for SQL-query datasources ignore user-dependent Jinja/ExtraCache context. If a query uses user-scoped macros (for example current-user values) and result caching is enabled, different users can collide on the same cache key and receive each other’s cached results. Build extra cache keys from the query SQL/template context (like SqlaTable does) so user-specific inputs are part of the cache key. [cache]
Severity Level: Critical 🚨
- ❌ Query-based charts with Jinja macros leak user-specific data.
- ❌ Cached /api/v1/chart/data results shared between different users.
- ⚠️ Query datasources ignore ExtraCache user context entirely.
- ⚠️ Per-user caching relies solely on global impersonation flags.Steps of Reproduction ✅
1. In `superset/common/query_context_processor.py:59-77`,
`QueryContextProcessor.query_cache_key` calls
`datasource.get_extra_cache_keys(query_obj.to_dict())` and passes the result into
`query_obj.cache_key(...)` as the `extra_cache_keys` argument.
2. When a chart uses a SQL Lab saved query as its datasource (datasource type
`DatasourceType.QUERY`), `self._qc_datasource` in `QueryContextProcessor` is an instance
of `Query` from `superset/models/sql_lab.py:16-27`.
3. The `Query` class implementation of `get_extra_cache_keys` at
`superset/models/sql_lab.py:369-370` simply returns an empty list, so `extra_cache_keys`
passed into `QueryObject.cache_key` (see `superset/common/query_object.py:489-500`) is
always `[]` for query-based datasources.
4. Create a SQL Lab query that uses user-scoped Jinja macros (for example `{{
current_user_id() }}` or `{{ url_param('tenant_id') }}` from
`superset/jinja_context.py:46-60, 240-37`), save it, and build a chart on top of this
saved query; then enable result caching and hit `/api/v1/chart/data` as two different
users. Because `Query.get_extra_cache_keys` ignores any user-specific context, both users
share the same cache key and the second user receives the first user’s cached results
despite different macro inputs.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/sql_lab.py
**Line:** 369:370
**Comment:**
*Cache: Returning an unconditional empty list here makes cache keys for SQL-query datasources ignore user-dependent Jinja/ExtraCache context. If a query uses user-scoped macros (for example current-user values) and result caching is enabled, different users can collide on the same cache key and receive each other’s cached results. Build extra cache keys from the query SQL/template context (like SqlaTable does) so user-specific inputs are part of the cache key.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix2 participants
SUMMARY
Fixes #37507: Chart data API now returns per-phase timing breakdown.
Before: Timing computed server-side → sent to
STATS_LOGGER→ discarded by default (DummyStatsLogger). No timing in API response.After:
timingobject included in response with per-phase breakdown, controlled byCHART_DATA_INCLUDE_TIMINGconfig.Problem Analysis
Root Cause (from issue #37507)
/api/v1/chart/datadoes not@statsd_metricscomputes timing → sends toSTATS_LOGGER→ not in responseDummyStatsLoggerdiscards alltiming()callsKey Code Location
QueryContextProcessor.get_df_payload()insuperset/common/query_context_processor.py— the main query lifecycle method that handles validation, cache lookup, DB execution, and result processing.Solution: Per-Phase Timing Instrumentation
Architecture
Wrap each phase in
get_df_payload()withtime.perf_counter():Configuration
CHART_DATA_INCLUDE_TIMINGFalseFalse, timing is still collected forSTATS_LOGGERand slow-query logging but omitted from the response.CHART_DATA_SLOW_QUERY_THRESHOLD_MSNoneResponse Format
When
CHART_DATA_INCLUDE_TIMING = True:{ "result": [{ "cache_key": "...", "data": [...], "timing": { "validate_ms": 2.1, "cache_lookup_ms": 5.3, "db_execution_ms": 850.0, "result_processing_ms": 12.5, "total_ms": 870.1, "is_cached": false } }] }db_execution_msisnullon cache hit, numeric on cache miss>= 0, rounded to 2 decimal placesCHART_DATA_INCLUDE_TIMING = False(default), thetimingkey is absent from the responseSlow Query Logging
When
CHART_DATA_SLOW_QUERY_THRESHOLD_MSis set, queries exceeding the threshold emit WARNING with full timing breakdown (independent ofCHART_DATA_INCLUDE_TIMING):Per-Phase Metrics
Each phase emitted to
STATS_LOGGER.timing()in milliseconds (independent ofCHART_DATA_INCLUDE_TIMING):chart_data.validate_ms,chart_data.cache_lookup_ms, etc.Changes Summary
superset/common/query_context_processor.pytime.perf_counter(), emit metrics, slow query log, config-gated responsesuperset/common/query_actions.pytimingthrough RESULTS result_typesuperset/config.pyCHART_DATA_INCLUDE_TIMINGandCHART_DATA_SLOW_QUERY_THRESHOLD_MSconfigssuperset/charts/schemas.pyChartDataTimingSchemawithdb_execution_msasallow_none=Truetests/unit_tests/common/test_query_context_processor_timing.pyNew Tests
test_timing_present_in_payloadtimingdict exists in result when config enabledtest_timing_omitted_when_config_disabledtimingabsent whenCHART_DATA_INCLUDE_TIMING = Falsetest_timing_values_are_non_negativetest_timing_no_db_execution_on_cache_hitdb_execution_msisNoneon cache hittest_timing_has_db_execution_on_cache_missdb_execution_mspresent on cache misstest_slow_query_loggingBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (API-only change, no UI)
TESTING INSTRUCTIONS
CHART_DATA_INCLUDE_TIMING = Trueinsuperset_config.py/api/v1/chart/dataresponsetimingfieldtimingobject with phase breakdownADDITIONAL INFORMATION
Related discussions: #18431, #13044