fix(views): emit deprecated-endpoint log warning once per endpoint per process#41286
fix(views): emit deprecated-endpoint log warning once per endpoint per process#41286eschutho wants to merge 2 commits into
Conversation
The `deprecated` decorator in views/base.py logged a WARNING on every HTTP request to any deprecated endpoint. In production, `explore_json` is called by legacy chart types on every chart render, flooding logs with the same message thousands of times per day and making the signal meaningless. Add a per-endpoint `_warned` flag (closure variable inside `_deprecated`) so the warning fires exactly once per endpoint per worker process. Subsequent requests to the same deprecated endpoint are handled silently. The warning message and format are unchanged; only the repetition is eliminated. A benign first-call race on multi-threaded workers may emit two or three log lines per worker on startup, which is acceptable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
the
logging
Code Review Agent Run #d1ac83Actionable 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 |
…vior Self-review (019ef022-d140-77b3-bd7a-6e32a74ea00b) required a test to guard the _warned closure flag introduced in the prior commit. Verifies that calling a @deprecated-decorated endpoint twice emits exactly one logger.warning call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ 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}
| nonlocal _warned | ||
| if not _warned: | ||
| _warned = True | ||
| message = ( | ||
| "%s.%s " | ||
| "This API endpoint is deprecated and will be removed in version %s" | ||
| ) | ||
| logger_args = [ | ||
| self.__class__.__name__, | ||
| f.__name__, | ||
| eol_version, | ||
| ] | ||
| if new_target: | ||
| message += " . Use the following API endpoint instead: %s" | ||
| logger_args.append(new_target) | ||
| logger.warning(message, *logger_args) |
There was a problem hiding this comment.
Suggestion: The new one-time warning guard is not thread-safe: concurrent first requests can both pass the if not _warned check before either write is visible to the other, so the warning can still be emitted multiple times in a worker. This violates the intended "once per endpoint per process" behavior; guard the check/update with a lock (or another atomic one-time primitive) around the warning block. [race condition]
Severity Level: Minor 🧹
- ⚠️ Deprecated explore_json logs may duplicate under concurrent load.
- ⚠️ Deprecation logging not strictly once-per-process as intended.Steps of Reproduction ✅
1. Observe the deprecated decorator implementation in `superset/views/base.py` at lines
15–48 (Read output), specifically the inner `wraps` function where the guard is
implemented (diff hunk lines 176–193): it uses `nonlocal _warned`, checks `if not
_warned:`, then sets `_warned = True` and emits `logger.warning(...)` without any
synchronization.
2. Confirm that a real HTTP endpoint uses this decorator: in `superset/views/core.py`
lines 280–319, the `explore_json` view is exposed via `@expose("/explore_json/",
methods=("GET","POST"))` and
`@expose("/explore_json/<datasource_type>/<int:datasource_id>/", ...)`, and is decorated
with `@deprecated(eol_version="5.0.0", new_target="/api/v1/chart/data")` at line 18 (Read
output), so every call to `/explore_json` passes through the `_warned` guard in
`superset/views/base.py`.
3. Run Superset with this PR code under a multi-threaded WSGI server (e.g., gunicorn
worker configured with `--threads > 1`), so that multiple requests to `/explore_json` can
be processed concurrently by the same process and thus share the same `_warned` closure
defined in `deprecated` at `superset/views/base.py:24–46`.
4. Immediately after starting a worker, send two or more concurrent HTTP requests to
`/explore_json` (or `/explore_json/<datasource_type>/<datasource_id>/`) so they reach the
same process at nearly the same time; due to the unsynchronized `if not _warned: _warned =
True` sequence in `superset/views/base.py` lines 28–31, two threads can both see `_warned`
as `False` before either write is visible, causing the warning block to execute in both
threads and resulting in multiple `"This API endpoint is deprecated and will be removed in
version %s"` log entries instead of a single once-per-process warning.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 177:192
**Comment:**
*Race Condition: The new one-time warning guard is not thread-safe: concurrent first requests can both pass the `if not _warned` check before either write is visible to the other, so the warning can still be emitted multiple times in a worker. This violates the intended "once per endpoint per process" behavior; guard the check/update with a lock (or another atomic one-time primitive) around the warning block.
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41286 +/- ##
==========================================
- Coverage 64.36% 63.76% -0.60%
==========================================
Files 2653 2653
Lines 144867 144965 +98
Branches 33421 33452 +31
==========================================
- Hits 93250 92444 -806
- Misses 49946 50836 +890
- Partials 1671 1685 +14
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:
|
Code Review Agent Run #6979adActionable 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 |
1 participant
Summary
Deprecation warning found in production Datadog:
Superset.explore_json This API endpoint is deprecated and will be removed in version 5.0.0This message fired on every HTTP request to any endpoint decorated with
@deprecatedinviews/base.py. Theexplore_jsonendpoint — still used by legacy chart types withuseLegacyApi: true— was generating this warning thousands of times per day, making the log signal meaningless and adding noise to Datadog.What changed:
_warned = Falseclosure variable inside_deprecated(one flag per decorated endpoint, allocated at decoration time).No behavior change: the endpoint continues to respond normally on every request. Only the repeated log line is suppressed.
Thread safety: the first-call race on multi-threaded workers is benign — at worst two or three log lines on startup, then silence. No lock needed.
Test plan
explore_jsondeprecation warning appears exactly once in logs after a gunicorn worker starts (not on every request)pytest tests/integration_tests/core_tests.py -k explore_jsongrep -rn "deprecated.*Warning" tests/returns no hits forviews/base)🤖 Generated with Claude Code