Conversation
Add server query functions to the ProxySQL client (GetServers, GetServersByHostgroup) and expose them as HTTP API endpoints at /api/proxysql/servers and /api/proxysql/servers/:hostgroup. - New go/proxysql/topology.go with ServerEntry struct and query methods - New go/proxysql/topology_test.go with struct and nil-client tests - GetClient() accessor on Hook for use by HTTP handlers - API endpoints registered in go/http/api.go - Documentation updated in docs/proxysql-hooks.md Closes #31
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new ProxySQL topology query API, providing HTTP endpoints to retrieve detailed information about ProxySQL's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a new API for querying ProxySQL server topology, which is a valuable addition for monitoring and debugging. The changes include new API endpoints, corresponding data access logic in the proxysql package, and documentation. The implementation is solid. I've provided a couple of suggestions to reduce code duplication and improve maintainability in the new API handlers and data access functions. Overall, this is a great enhancement.
| hook := proxysql.GetHook() | ||
| if hook == nil || !hook.IsConfigured() { | ||
| Respond(r, &APIResponse{Code: ERROR, Message: "ProxySQL is not configured"}) | ||
| return |
There was a problem hiding this comment.
The logic to get the ProxySQL hook and check if it's configured is duplicated here and in the ProxySQLServersByHostgroup function. To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method on HttpAPI.
For example, you could create a helper like this:
func (this *HttpAPI) getProxySQLClient(r render.Render) *proxysql.Client {
hook := proxysql.GetHook()
if hook == nil || !hook.IsConfigured() {
Respond(r, &APIResponse{Code: ERROR, Message: "ProxySQL is not configured"})
return nil
}
return hook.GetClient()
}Then, both ProxySQLServers and ProxySQLServersByHostgroup can call this helper at the beginning:
func (this *HttpAPI) ProxySQLServers(params martini.Params, r render.Render, req *http.Request) {
client := this.getProxySQLClient(r)
if client == nil {
return
}
// ... rest of the function
}| func (c *Client) GetServers() ([]ServerEntry, error) { | ||
| if c == nil { | ||
| return []ServerEntry{}, nil | ||
| } | ||
|
|
||
| query := "SELECT hostgroup_id, hostname, port, status, weight, max_connections, comment FROM runtime_mysql_servers ORDER BY hostgroup_id, hostname, port" | ||
| rows, db, err := c.Query(query) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServers: %v", err) | ||
| } | ||
| defer db.Close() | ||
| defer rows.Close() | ||
|
|
||
| var servers []ServerEntry | ||
| for rows.Next() { | ||
| var s ServerEntry | ||
| if err := rows.Scan(&s.HostgroupID, &s.Hostname, &s.Port, &s.Status, &s.Weight, &s.MaxConns, &s.Comment); err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServers scan: %v", err) | ||
| } | ||
| servers = append(servers, s) | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServers rows: %v", err) | ||
| } | ||
|
|
||
| log.Infof("proxysql: GetServers returned %d entries", len(servers)) | ||
| return servers, nil | ||
| } | ||
|
|
||
| // GetServersByHostgroup returns servers filtered by hostgroup ID. | ||
| // Returns an empty slice (no error) if the client is nil. | ||
| func (c *Client) GetServersByHostgroup(hostgroupID int) ([]ServerEntry, error) { | ||
| if c == nil { | ||
| return []ServerEntry{}, nil | ||
| } | ||
|
|
||
| query := "SELECT hostgroup_id, hostname, port, status, weight, max_connections, comment FROM runtime_mysql_servers WHERE hostgroup_id = ? ORDER BY hostname, port" | ||
| rows, db, err := c.Query(query, hostgroupID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServersByHostgroup(%d): %v", hostgroupID, err) | ||
| } | ||
| defer db.Close() | ||
| defer rows.Close() | ||
|
|
||
| var servers []ServerEntry | ||
| for rows.Next() { | ||
| var s ServerEntry | ||
| if err := rows.Scan(&s.HostgroupID, &s.Hostname, &s.Port, &s.Status, &s.Weight, &s.MaxConns, &s.Comment); err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServersByHostgroup scan: %v", err) | ||
| } | ||
| servers = append(servers, s) | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServersByHostgroup rows: %v", err) | ||
| } | ||
|
|
||
| log.Infof("proxysql: GetServersByHostgroup(%d) returned %d entries", hostgroupID, len(servers)) | ||
| return servers, nil | ||
| } |
There was a problem hiding this comment.
The logic for iterating through *sql.Rows and scanning them into ServerEntry structs is duplicated in GetServers and GetServersByHostgroup. You can extract this into a helper function to avoid repetition and make the code more modular. This will improve the maintainability of this new file.
func (c *Client) GetServers() ([]ServerEntry, error) {
if c == nil {
return []ServerEntry{}, nil
}
query := "SELECT hostgroup_id, hostname, port, status, weight, max_connections, comment FROM runtime_mysql_servers ORDER BY hostgroup_id, hostname, port"
rows, db, err := c.Query(query)
if err != nil {
return nil, fmt.Errorf("proxysql: GetServers: %v", err)
}
defer db.Close()
defer rows.Close()
servers, err := scanServerEntries(rows)
if err != nil {
return nil, fmt.Errorf("proxysql: GetServers: failed to process rows: %v", err)
}
log.Infof("proxysql: GetServers returned %d entries", len(servers))
return servers, nil
}
// GetServersByHostgroup returns servers filtered by hostgroup ID.
// Returns an empty slice (no error) if the client is nil.
func (c *Client) GetServersByHostgroup(hostgroupID int) ([]ServerEntry, error) {
if c == nil {
return []ServerEntry{}, nil
}
query := "SELECT hostgroup_id, hostname, port, status, weight, max_connections, comment FROM runtime_mysql_servers WHERE hostgroup_id = ? ORDER BY hostname, port"
rows, db, err := c.Query(query, hostgroupID)
if err != nil {
return nil, fmt.Errorf("proxysql: GetServersByHostgroup(%d): %v", hostgroupID, err)
}
defer db.Close()
defer rows.Close()
servers, err := scanServerEntries(rows)
if err != nil {
return nil, fmt.Errorf("proxysql: GetServersByHostgroup: failed to process rows: %v", err)
}
log.Infof("proxysql: GetServersByHostgroup(%d) returned %d entries", hostgroupID, len(servers))
return servers, nil
}
// scanServerEntries is a helper to scan rows into a slice of ServerEntry.
func scanServerEntries(rows *sql.Rows) ([]ServerEntry, error) {
var servers []ServerEntry
for rows.Next() {
var s ServerEntry
if err := rows.Scan(&s.HostgroupID, &s.Hostname, &s.Port, &s.Status, &s.Weight, &s.MaxConns, &s.Comment); err != nil {
return nil, err
}
servers = append(servers, s)
}
return servers, rows.Err()
}There was a problem hiding this comment.
Pull request overview
Adds a ProxySQL topology query surface to orchestrator by extending the ProxySQL client with runtime_mysql_servers queries and exposing those results via new HTTP API endpoints.
Changes:
- Add
ServerEntryplusGetServers()/GetServersByHostgroup()to the ProxySQL client package. - Add HTTP endpoints
GET /api/proxysql/serversandGET /api/proxysql/servers/:hostgroupto return ProxySQL server entries. - Document the new topology endpoints in
docs/proxysql-hooks.mdand add aHook.GetClient()accessor for the HTTP layer.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go/proxysql/topology.go | Introduces a server row model and query helpers against runtime_mysql_servers. |
| go/proxysql/topology_test.go | Adds basic unit tests for the new struct and nil-client behavior. |
| go/proxysql/hook.go | Exposes the underlying ProxySQL client via a new accessor. |
| go/http/api.go | Registers and implements new REST endpoints to fetch ProxySQL server topology. |
| docs/proxysql-hooks.md | Documents the new topology query endpoints and sample responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetClient returns the underlying ProxySQL client, or nil if the hook is not configured. | ||
| func (h *Hook) GetClient() *Client { | ||
| if h == nil { | ||
| return nil | ||
| } | ||
| return h.client | ||
| } |
There was a problem hiding this comment.
The GetClient() docstring says it returns nil when the hook is not configured, but the implementation only checks h == nil and otherwise returns h.client even when writerHostgroup is 0 (i.e., IsConfigured() is false). Either update the comment to reflect the actual behavior, or change GetClient() to return nil when !h.IsConfigured() to avoid misleading callers.
| hook := proxysql.GetHook() | ||
| if hook == nil || !hook.IsConfigured() { | ||
| Respond(r, &APIResponse{Code: ERROR, Message: "ProxySQL is not configured"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
These endpoints treat ProxySQL as “not configured” unless hook.IsConfigured() is true, but IsConfigured() currently requires ProxySQLWriterHostgroup > 0. That prevents using the topology query API when only the ProxySQL Admin connection is configured (address/user/pass) but failover hooks are intentionally disabled. Consider gating on hook.GetClient() != nil (or adding a dedicated “admin client configured” check) so read-only topology queries can work without writer hostgroup configuration.
| "github.com/proxysql/golib/log" | ||
| ) | ||
|
|
||
| // ServerEntry represents a row from ProxySQL's mysql_servers table. |
There was a problem hiding this comment.
The struct comment says ServerEntry represents a row from mysql_servers, but the new query methods read from runtime_mysql_servers. Please update the comment to match the actual source table (or mention both if intended).
| // ServerEntry represents a row from ProxySQL's mysql_servers table. | |
| // ServerEntry represents a row from ProxySQL's runtime_mysql_servers table | |
| // (the runtime view of the mysql_servers configuration). |
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("proxysql: GetServers rows: %v", err) | ||
| } | ||
|
|
||
| log.Infof("proxysql: GetServers returned %d entries", len(servers)) | ||
| return servers, nil |
There was a problem hiding this comment.
GetServers/GetServersByHostgroup log at Info level on every call. Since these methods may be invoked frequently via the new HTTP endpoints (polling/monitoring), this can create noisy logs in production. Consider lowering this to a debug-level log or removing it (or rate-limiting) to avoid log volume issues.
Summary
ServerEntrystruct andGetServers()/GetServersByHostgroup()query methods to the ProxySQL client packageGET /api/proxysql/serversandGET /api/proxysql/servers/:hostgroupGetClient()accessor toHookfor use by the HTTP layerdocs/proxysql-hooks.mdCloses #31
Test plan
go buildsucceedsgo test ./go/proxysql/... -vpasses (10 tests including new topology tests)