feat(bigtable): debugview + tcpz stack, gated on ClientConfig.EnableDebug - #20292
feat(bigtable): debugview + tcpz stack, gated on ClientConfig.EnableDebug#20292sushanb wants to merge 2 commits into
Conversation
product-auto-label
Bot
added
the
api: bigtable
![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 comprehensive diagnostic debug view package (debugview) for the Cloud Bigtable Go client, adding endpoints like sessionz, afez, loadz, channelz, configz, tcpz, and debugtagsz to expose live internal client state. The review feedback highlights critical safety and robustness improvements, including handling time.MinDuration in duration rounding functions to prevent infinite recursion crashes, using url.PathEscape in manually constructed HTML links to avoid broken routing, and consistently utilizing the shared writeJSON and writeHTML helpers across the new handlers to improve code reuse and avoid duplicate header warnings.
| func roundDurationShort(d time.Duration) time.Duration { | ||
| switch { | ||
| case d < 0: | ||
| return -roundDurationShort(-d) | ||
| case d == 0: | ||
| return 0 | ||
| case d < time.Millisecond: | ||
| return d.Round(time.Microsecond) | ||
| case d < time.Second: | ||
| return d.Round(time.Millisecond) | ||
| default: | ||
| return d.Round(10 * time.Millisecond) | ||
| } | ||
| } |
There was a problem hiding this comment.
If d is time.MinDuration, d < 0 is true, but -d overflows and remains time.MinDuration. This causes infinite recursion and a stack overflow, which immediately crashes the Go process. We should handle time.MinDuration safely to prevent potential crashes.
| func roundDurationShort(d time.Duration) time.Duration { | |
| switch { | |
| case d < 0: | |
| return -roundDurationShort(-d) | |
| case d == 0: | |
| return 0 | |
| case d < time.Millisecond: | |
| return d.Round(time.Microsecond) | |
| case d < time.Second: | |
| return d.Round(time.Millisecond) | |
| default: | |
| return d.Round(10 * time.Millisecond) | |
| } | |
| } | |
| func roundDurationShort(d time.Duration) time.Duration { | |
| if d < 0 { | |
| if d == time.MinDuration { | |
| return time.MinDuration | |
| } | |
| return -roundDurationShort(-d) | |
| } | |
| switch { | |
| case d == 0: | |
| return 0 | |
| case d < time.Millisecond: | |
| return d.Round(time.Microsecond) | |
| case d < time.Second: | |
| return d.Round(time.Millisecond) | |
| default: | |
| return d.Round(10 * time.Millisecond) | |
| } | |
| } |
| func roundDurationLong(d time.Duration) time.Duration { | ||
| switch { | ||
| case d > time.Hour: | ||
| return d.Round(time.Minute) | ||
| case d > time.Minute: | ||
| return d.Round(time.Second) | ||
| case d > time.Second: | ||
| return d.Round(10 * time.Millisecond) | ||
| default: | ||
| return d.Round(time.Microsecond) | ||
| } | ||
| } |
There was a problem hiding this comment.
roundDurationLong does not handle negative durations symmetrically (it falls back to microsecond rounding), unlike roundDurationShort. We should make it symmetric and safe for time.MinDuration as well.
| func roundDurationLong(d time.Duration) time.Duration { | |
| switch { | |
| case d > time.Hour: | |
| return d.Round(time.Minute) | |
| case d > time.Minute: | |
| return d.Round(time.Second) | |
| case d > time.Second: | |
| return d.Round(10 * time.Millisecond) | |
| default: | |
| return d.Round(time.Microsecond) | |
| } | |
| } | |
| func roundDurationLong(d time.Duration) time.Duration { | |
| if d < 0 { | |
| if d == time.MinDuration { | |
| return time.MinDuration | |
| } | |
| return -roundDurationLong(-d) | |
| } | |
| switch { | |
| case d > time.Hour: | |
| return d.Round(time.Minute) | |
| case d > time.Minute: | |
| return d.Round(time.Second) | |
| case d > time.Second: | |
| return d.Round(10 * time.Millisecond) | |
| default: | |
| return d.Round(time.Microsecond) | |
| } | |
| } |
| poolEsc := template.HTMLEscapeString(r.PoolName) | ||
| nameEsc := template.HTMLEscapeString(r.LogName) | ||
| b.WriteString(`<a href="../sessionz/pool/`) | ||
| b.WriteString(poolEsc) | ||
| b.WriteString(`#`) | ||
| b.WriteString(nameEsc) | ||
| b.WriteString(`" title="jump to `) | ||
| b.WriteString(nameEsc) | ||
| b.WriteString(` in `) | ||
| b.WriteString(poolEsc) | ||
| b.WriteString(`">`) | ||
| b.WriteString(nameEsc) | ||
| b.WriteString(`</a>`) |
There was a problem hiding this comment.
Since this HTML is constructed manually and returned as template.HTML, it bypasses html/template's automatic contextual escaping. If r.PoolName or r.LogName contains special characters like ?, #, %, or spaces, the generated links will be broken or misrouted. We should use url.PathEscape for the URL path and fragment while keeping the unescaped (but HTMLEscaped) versions for the user-visible text and tooltips. Note that you will need to import "net/url" in this file.
poolPath := template.HTMLEscapeString(url.PathEscape(r.PoolName))
namePath := template.HTMLEscapeString(url.PathEscape(r.LogName))
poolHTML := template.HTMLEscapeString(r.PoolName)
nameHTML := template.HTMLEscapeString(r.LogName)
b.WriteString("<a href=\"../sessionz/pool/")
b.WriteString(poolPath)
b.WriteString("#")
b.WriteString(namePath)
b.WriteString(\"\" title=\"jump to \")
b.WriteString(nameHTML)
b.WriteString(\" in \")
b.WriteString(poolHTML)
b.WriteString(\">\")
b.WriteString(nameHTML)
b.WriteString(\"</a>\")| if r.URL.Query().Get("format") == "json" { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| enc := json.NewEncoder(w) | ||
| enc.SetIndent("", " ") | ||
| _ = enc.Encode(pools) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| // If Execute errors partway through, the body has already been | ||
| // (partially) written and the status code is committed to 200 — | ||
| // calling http.Error here would trigger a "superfluous | ||
| // WriteHeader" warning without doing anything useful for the | ||
| // client. Silently swallow; the partial page is what the caller | ||
| // gets to see. | ||
| _ = channelzTpl.Execute(w, channelzPageData{ | ||
| Pools: pools, | ||
| Generated: time.Now(), | ||
| HasProvider: s.provider != nil, | ||
| }) |
There was a problem hiding this comment.
For consistency and robustness, we should use the shared writeJSON and writeHTML helpers defined in helpers.go instead of manually setting headers and executing the template.
| if r.URL.Query().Get("format") == "json" { | |
| w.Header().Set("Content-Type", "application/json") | |
| enc := json.NewEncoder(w) | |
| enc.SetIndent("", " ") | |
| _ = enc.Encode(pools) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | |
| // If Execute errors partway through, the body has already been | |
| // (partially) written and the status code is committed to 200 — | |
| // calling http.Error here would trigger a "superfluous | |
| // WriteHeader" warning without doing anything useful for the | |
| // client. Silently swallow; the partial page is what the caller | |
| // gets to see. | |
| _ = channelzTpl.Execute(w, channelzPageData{ | |
| Pools: pools, | |
| Generated: time.Now(), | |
| HasProvider: s.provider != nil, | |
| }) | |
| if r.URL.Query().Get("format") == "json" { | |
| writeJSON(w, pools) | |
| return | |
| } | |
| writeHTML(w, channelzTpl, channelzPageData{ | |
| Pools: pools, | |
| Generated: time.Now(), | |
| HasProvider: s.provider != nil, | |
| }) |
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| if err := configzTpl.Execute(w, configzPageData{ | ||
| HasProvider: s.provider != nil, | ||
| Snapshot: snap, | ||
| ConfigJSON: jsonStr, | ||
| }); err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| } |
There was a problem hiding this comment.
We should use the shared writeHTML helper instead of manually setting headers and executing the template. This also avoids potential superfluous WriteHeader warnings if Execute fails after writing some bytes.
writeHTML(w, configzTpl, configzPageData{
HasProvider: s.provider != nil,
Snapshot: snap,
ConfigJSON: jsonStr,
})| if q.Get("format") == "json" { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Header().Set("Cache-Control", "no-store") | ||
| _ = json.NewEncoder(w).Encode(raw) | ||
| return | ||
| } |
There was a problem hiding this comment.
We should use the shared writeJSON helper instead of manually setting headers and encoding the JSON.
if q.Get("format") == "json" {
writeJSON(w, raw)
return
}| tpl := tcpzTpl | ||
| if flat { | ||
| tpl = tcpzFlatTpl | ||
| } | ||
| if err := tpl.Execute(w, data); err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| } |
There was a problem hiding this comment.
We should use the shared writeHTML helper instead of manually executing the template. This also avoids potential superfluous WriteHeader warnings if Execute fails after writing some bytes.
tpl := tcpzTpl
if flat {
tpl = tcpzFlatTpl
}
writeHTML(w, tpl, data)…ebug
Ports the 7-page /debug/ UI + per-connection TCP_INFO collector from
a downstream feature branch onto upstream/main. Wire the entire tree
with a one-liner:
client, _ := bigtable.NewClientWithConfig(ctx, "p", "i",
bigtable.ClientConfig{EnableDebug: true})
http.Handle("/debug/", http.StripPrefix("/debug",
debugview.Handler(client)))
Public API
- ClientConfig.EnableDebug bool — opts a Client into the TCPStats
collector. Zero cost when false: no dial hook, no allocation.
When true, NewClientWithConfig constructs TCPStats and appends
its dial option so every gRPC dial (classic + session pools)
registers with the collector.
- Client.SessionDebug / ChannelDebug / ConfigDebug / TCPStats
accessors — hand these directly to debugview.Handler.
- Type aliases in bigtable/session_debug.go re-export the debug
provider interfaces + snapshot types from internal/transport so
debugview depends only on the public bigtable package.
New packages
- bigtable/debugview/ — 8 z-page HTTP handlers (sessionz, afez,
loadz, channelz, configz, tcpz, debugtagsz) behind a single
Handler(client) entry. Every view supports ?format=json.
Nil-safe: panels render a "not enabled" banner when the
corresponding accessor returns nil.
- bigtable/internal/transport/conn_registry.go +
tcp_info_{linux,other}.go — the collector's backing store and
OS-specific TCP_INFO scrape via getsockopt.
- bigtable/tcp_stats.go — public TCPStats type + NewTCPStats()
constructor + ClientOption() returning
option.WithGRPCDialOption(grpc.WithContextDialer(...)).
Wiring
- client.go — new tcpStats field on Client; NewClientWithConfig
constructs it when config.EnableDebug is true, appends the
ClientOption to the dial option list, threads config.EnableDebug
into session.NewClient.
- internal/session/client.go — NewClient signature grows an
enableDebug bool that populates Config.EnableDebug on the
session-side pool.
Session-aware channel pool sizing is intentionally out of scope for
this PR — that stack lands separately.
…n prior commit) The debugview commit staged bigtable/client.go's new session.NewClient call signature but didn't stage the session-side signature bump. Adds the missing 'enableDebug bool' parameter and Config.EnableDebug propagation so the package compiles standalone.
3ee4b11 to
e10064d
Compare
1 participant
Summary
Ports the 7-page bigtable debug UI + per-connection TCP_INFO collector from a downstream feature branch onto `upstream/main`. Wire the entire tree with a one-liner:
```go
client, _ := bigtable.NewClientWithConfig(ctx, "p", "i",
bigtable.ClientConfig{EnableDebug: true})
http.Handle("/debug/", http.StripPrefix("/debug",
debugview.Handler(client)))
```
This is the debugview-only slice of #20275 — the SessionAwareChannelPool stack (`session_aware_scale_monitor.go`, `ClientConfigurationManager.AddChannelPoolConfigListener`, `BigtableChannelPool.TotalStreamCount`) is intentionally excluded and will land as a follow-up.
Public API additions
New packages / files
Wiring
What is NOT in this PR
Test plan