fix(tsdb): serve dashboard from REST API port for same-origin fetch (#5684)#5775
Conversation
…5684) The TSDB dashboard HTML was served from admin-web_port (default 6080) but its JS issues relative-URL fetches: <script src="/Chart.bundle.js"></script> fetch('/api/tsdb/metrics') fetch('/api/tsdb/query?...') Those URLs resolve against the page origin, so the browser tried to read /api/tsdb/* from :6080 -- but the endpoints live on admin-restapi_port (default 6070). Result: every fetch 404'd and the dashboard rendered "Error loading metrics". Move the dashboard and its Chart.bundle.js asset to the REST API server so the dashboard's relative URLs are same-origin with the endpoints they need. This is the smallest change that makes the feature actually work; backwards-compatibility isn't a constraint since the feature was completely broken at :6080. * lib/ProxySQL_RESTAPI_Server.cpp: register /tsdb and /Chart.bundle.js resources alongside the existing /api/tsdb/* handlers. * lib/ProxySQL_HTTP_Server.cpp: remove the now-redundant /tsdb route. Chart.bundle.js stays here too because other web-port dashboards (e.g. /, /stats) still reference it. * test/tap/tests/test_tsdb_api-t.cpp: rewrite the existing test to pin the new wiring. In particular, parse the live dashboard HTML, extract every src/href/fetch URL, and GET each one against the dashboard's origin -- this assertion fails on the pre-fix code and passes after. Verified locally: bug build fails 5/10 assertions, fixed build passes 10/10. User-facing change: the dashboard URL moves from http://host:admin-web_port/tsdb (broken) to http://host:admin-restapi_port/tsdb (working) The dashboard now requires admin-restapi_enabled=1 instead of admin-web_enabled=1; in practice both were required for the dashboard to be useful since the JS needed the API anyway.
📝 WalkthroughWalkthroughThe PR migrates TSDB dashboard serving from the HTTP server to the REST API server and substantially upgrades the regression test. The REST API server now declares and registers two new static asset endpoints ( ChangesTSDB Dashboard REST API Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the TSDB dashboard and its associated assets from the main HTTP server to the REST API server to resolve cross-origin issues when fetching metrics. The changes include the addition of new resource handlers in the REST API server and a comprehensive update to the test suite to verify same-origin resolution and JSON response integrity. Review feedback suggests including the header and using a safer lambda for string transformations to avoid potential undefined behavior with signed characters.
| #include <map> | ||
| #include <regex> |
There was a problem hiding this comment.
The header <cctype> should be included to provide the definition for std::tolower used in the header_cb function.
#include <cctype>
#include <map>
#include <regex>| if (colon != string::npos) { | ||
| string key = line.substr(0, colon); | ||
| string val = line.substr(colon + 1); | ||
| std::transform(key.begin(), key.end(), key.begin(), ::tolower); |
There was a problem hiding this comment.
Using ::tolower directly with std::transform on a std::string is a common pitfall. If char is signed, passing a negative value to tolower results in undefined behavior. It is safer to use a lambda that casts the input to unsigned char before calling std::tolower.
std::transform(key.begin(), key.end(), key.begin(), [](unsigned char c){ return std::tolower(c); });![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/test_tsdb_api-t.cpp`:
- Line 197: The test constructs web_origin with the wrong protocol (uses
"https://" for ProxySQL's admin web server); update the construction of
web_origin to use "http://" instead of "https://", e.g. build web_origin from
cl.admin_host and web_port using "http://" so subsequent checks (like the 404
check that hits the admin web port) use the correct protocol; verify usage of
web_origin in the test to ensure no other places assume HTTPS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d60f549f-03ac-489c-9632-9fde3b233b9a
📒 Files selected for processing (4)
include/ProxySQL_RESTAPI_Server.hpplib/ProxySQL_HTTP_Server.cpplib/ProxySQL_RESTAPI_Server.cpptest/tap/tests/test_tsdb_api-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables must use snake_case naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
include/ProxySQL_RESTAPI_Server.hpplib/ProxySQL_HTTP_Server.cpplib/ProxySQL_RESTAPI_Server.cpptest/tap/tests/test_tsdb_api-t.cpp
**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in header files must use
#ifndef __CLASS_*_Hpattern
Files:
include/ProxySQL_RESTAPI_Server.hpp
test/tap/tests/{test_*,-t}.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must follow the naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/test_tsdb_api-t.cpp
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test binaries are built via pattern rule in
test/tap/tests/Makefile:make <testname>-tcompiles<testname>-t.cppinto<testname>-twithout requiring explicit Makefile targets
Files:
test/tap/tests/test_tsdb_api-t.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_tsdb_api-t.cpp
🔇 Additional comments (13)
include/ProxySQL_RESTAPI_Server.hpp (1)
18-19: LGTM!lib/ProxySQL_RESTAPI_Server.cpp (3)
460-464: LGTM!
466-482: LGTM!
542-549: LGTM!lib/ProxySQL_HTTP_Server.cpp (1)
440-443: LGTM!test/tap/tests/test_tsdb_api-t.cpp (8)
1-22: LGTM!
44-54: LGTM!
56-70: LGTM!
72-100: LGTM!
102-109: LGTM!
114-124: LGTM!
126-189: LGTM!
202-284: LGTM!
| sleep(8); | ||
|
|
||
| const string restapi_origin = string("http://") + cl.admin_host + ":" + std::to_string(restapi_port); | ||
| const string web_origin = string("https://") + cl.admin_host + ":" + std::to_string(web_port); |
There was a problem hiding this comment.
Incorrect protocol for web_origin.
Line 197 uses https:// for the web server origin, but ProxySQL's admin web server (admin-web_port) serves HTTP by default, not HTTPS. This will cause connection failures when the test attempts to verify the dashboard returns 404 on the web port (line 230).
🔧 Proposed fix
- const string web_origin = string("https://") + cl.admin_host + ":" + std::to_string(web_port);
+ const string web_origin = string("http://") + cl.admin_host + ":" + std::to_string(web_port);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const string web_origin = string("https://") + cl.admin_host + ":" + std::to_string(web_port); | |
| const string web_origin = string("http://") + cl.admin_host + ":" + std::to_string(web_port); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tap/tests/test_tsdb_api-t.cpp` at line 197, The test constructs
web_origin with the wrong protocol (uses "https://" for ProxySQL's admin web
server); update the construction of web_origin to use "http://" instead of
"https://", e.g. build web_origin from cl.admin_host and web_port using
"http://" so subsequent checks (like the 404 check that hits the admin web port)
use the correct protocol; verify usage of web_origin in the test to ensure no
other places assume HTTPS.
|
07ae4fd
into
integration/v3.0-batch-2026-05-13
1 participant
Fixes #5684.
Summary
Chart.bundle.jsasset fromadmin-web_porttoadmin-restapi_port, so the dashboard's relative-URLfetch()calls are same-origin with the/api/tsdb/*endpoints they need./tsdbroute fromProxySQL_HTTP_Server.test_tsdb_api-t.cppto actually exercise the same-origin assumption -- it now parses the live dashboard HTML, extracts everysrc/href/fetchURL, andGETs each one against the dashboard's origin.Root cause
The dashboard HTML (
lib/TSDB_Dashboard_html.cpp) issues relative-URL requests:Browsers resolve those against the page origin. The dashboard was served from
ProxySQL_HTTP_Server(admin-web_port, default 6080) but/api/tsdb/*lives onProxySQL_RESTAPI_Server(admin-restapi_port, default 6070). Result: every fetch 404'd at:6080/api/tsdb/...and the dashboard rendered "Error loading metrics" in the UI -- exactly what #5684 reported.The user diagnosed this perfectly: "In the browser console, it seems to be trying to call the API on port 6080, but the API port is 6070 (admin-restapi_port)."
Considered alternatives
restapi_portinto the HTML + CORS on the APIweb_portinstead of the dashboard:6070/api/tsdb/*/api/tsdb/*from web_port to restapi_port:6080/tsdb+ 302 to:6070/tsdb:6080today, so there is no working URL to preserveFix
lib/ProxySQL_RESTAPI_Server.cpp: add two minimalhttp_resourcesubclasses (tsdb_dashboard_resource,tsdb_chart_js_resource) returningTSDB_Dashboard_html_candChart_bundle_js_cwith properContent-Type. Register them at/tsdband/Chart.bundle.jsalongside the existing/api/tsdb/*handlers.lib/ProxySQL_HTTP_Server.cpp: remove the/tsdbhandler and the now-unusedTSDB_Dashboard_html_cextern.Chart.bundle.jsstays here too because other web-port dashboards (/,/stats) reference it.include/ProxySQL_RESTAPI_Server.hpp: add twounique_ptr<http_resource>members to own the new resources.Verification
Local repro with
restapi_port=6071,web_port=6081,tsdb-enabled=1:Test plan
The existing
test/tap/tests/test_tsdb_api-t.cppwas rewritten to actually catch the bug. It now:admin-restapi_port/admin-web_port(honours non-default config)./tsdbis reachable on the REST API port and returns HTML.src=\"/...\",href=\"/...\",fetch(\"/...\"), andGETs each one against the dashboard's origin. This is the core TSDB dashboard is showing Error loading metrics #5684 assertion -- a 404 here means the dashboard is broken in a browser./api/tsdb/statusreturns a JSON object with the five documented keys./api/tsdb/metricsreturns a JSON array./api/tsdb/queryreturns rows with{ts, metric, labels, value}shape.Local verification:
PROXYSQL31=1(required forPROXYSQLTSDB).test_tsdb_api-tagainst a manually-configured ProxySQL withtsdb-enabled=1,admin-restapi_enabled=1,admin-web_enabled=1.ai-g1(wheretest_tsdb_api-tis registered).User-facing change
The dashboard URL moves from
http://host:admin-web_port/tsdb(broken) tohttp://host:admin-restapi_port/tsdb(working). The dashboard now requiresadmin-restapi_enabled=1instead ofadmin-web_enabled=1. In practice both flags had to be set for the dashboard to be useful (the JS always needed the API), so this is not a new requirement.Summary by CodeRabbit
New Features
Bug Fixes