Skip to content

feat(datasource): add opt-in per-datasource query rate limit - #43

Open
devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1789408704-datasource-query-rate-limit
Open

devin-ai-integration[bot] wants to merge 1 commit into
masterfrom
devin/1789408704-datasource-query-rate-limit

Conversation

@devin-ai-integration

Copy link
Copy Markdown

SUMMARY

Adds an opt-in, per-datasource query rate limit. When enabled, at most max_queries physical queries are executed against a single datasource per period_seconds window, across all users; the request that exceeds the budget fails with HTTP 429 (DATASOURCE_QUERY_RATE_LIMIT_ERROR, issue code 1042).

# superset_config.py
DATASOURCE_QUERY_RATE_LIMIT = {"enabled": True, "max_queries": 60, "period_seconds": 60}

Disabled by default, so existing behavior and existing tests are untouched.

Enforcement point: ExploreMixin.query() (superset/models/helpers.py)

def query(self, query_obj: QueryObjectDict) -> QueryResult:
    enforce_datasource_query_rate_limit(self.uid)
    ...
    df = self.database.get_df(sql, self.catalog, self.schema, mutator=...)

This is the single unified point where any explorable datasource (SqlaTable, SQL-Lab Query) turns a query object into SQL and hits the analytics database. Every consumer funnels through it — /api/v1/chart/data sync and Celery/async, dashboards, drill-by, samples, thumbnails, alerts & reports — so the limit can't be bypassed by a new API surface, and it counts physical database queries rather than HTTP requests: results served from the query cache in QueryContextProcessor.get_df_payload() correctly don't consume budget, since they never reach the database.

Rejected alternatives

  • Flask-Limiter on the route — new dependency, and it throttles per endpoint/IP/user with no idea which datasource is targeted. It also can't see async execution, where the HTTP request returns immediately and the query runs later in a Celery worker.
  • ChartDataCommand.validate() / the chart data API layer — runs before the cache lookup, so cache hits would be billed as queries, and it only covers chart data: SQL Lab, thumbnails, alerts and samples would bypass it entirely.
  • QueryContextProcessor.get_df_payload() — correct with respect to caching, but still chart-data-only; thumbnails, reports and other datasource consumers don't go through a query context.
  • Database.get_df() / the engine spec layer — no datasource identity is available at that depth (only a SQL string and a database), and it would also throttle unrelated traffic such as metadata introspection and SQL Lab autocomplete.

The counter is a fixed-window counter in the general-purpose cache (CACHE_CONFIG), keyed datasource-query-rate-limit:{uid}:{window} with TTL = window; a shared backend such as Redis is required for the limit to apply across workers. It is read and written without a lock, so under high concurrency the effective limit can slightly exceed the configured one — acceptable for protecting an analytics database from runaway load, and documented in the module docstring.

TESTING INSTRUCTIONS

pytest tests/unit_tests/common/test_query_rate_limit.py

Covers the allowed path (calls under the budget pass and increment the counter), the throttled path (the call over the budget raises a 429 SupersetRateLimitExceededException and never reaches database.get_df()), per-datasource isolation of budgets, and the disabled-by-default no-op.

Manually: set DATASOURCE_QUERY_RATE_LIMIT = {"enabled": True, "max_queries": 2, "period_seconds": 60} in superset_config.py, then load a chart with force=true three times within a minute — the third returns 429 with "Query rate limit exceeded for this datasource".

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

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/e67c29adf6ca4c43bc669e4baeee3a58
Open in Devin Desktop: https://app.devin.ai/desktop/session/e67c29adf6ca4c43bc669e4baeee3a58?variant=devin
Requested by: @patrickbradley-cog

Adds an opt-in, fixed-window rate limit on physical datasource queries,
enforced in ExploreMixin.query(). Disabled by default.

Co-Authored-By: patrick.bradley <patrick.bradley@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant