Hide the Workbench's own datastore queries from Top Queries - #390
Open
dpage wants to merge 3 commits into
Open
Conversation
The "Hide monitoring queries" toggle on the Server dashboard's Top Queries panel filtered on the ai_dba_wb_probe marker alone, which the collector wraps only around the read-only probe queries it runs against monitored databases. Because the metadata datastore normally shares a PostgreSQL instance with those databases, pg_stat_statements also captured the Workbench's own traffic against its own datastore, and none of it carried a marker: the collector's bulk metrics.* inserts, its partition maintenance and change-detection reads, and the alerter's metric-evaluation queries all leaked through the filter. Those statements are now tagged with an in-statement comment marker from a new shared pkg/sqlmarker package, applied at the few chokepoints each binary funnels its datastore traffic through rather than at the individual SQL literals, so that statements added later are tagged by construction. The server's filter excludes both markers. Placement of the marker is load-bearing and not obvious: PostgreSQL does not preserve a comment in every position when it normalises a statement for pg_stat_statements. Measured on PostgreSQL 18, a leading comment and a comment following the semicolon are both stripped, whilst a comment inside the statement survives; the marker therefore sits immediately after the leading keyword, and sqlmarker.Tag documents why so that nobody moves it to the front of the string and silently breaks the filter. The statements are tagged individually rather than excluding the metadata database wholesale, because users legitimately run other tools against that same data and expect to keep seeing them in the panel. Whilst here, the shared pkg module's tests now run from the root make test and test-all targets; no target ran them before, so the new package's tests would never have been exercised by the gate. Closes #364
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughIntroduces a shared SQL marker package, tags internal collector and alerter queries, refactors partition SQL behind an injectable interface, extends Top Queries filtering to internal traffic, and adds broad unit, integration, API, documentation, and test-target coverage. ChangesInternal SQL marker coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant handleTopQueries
participant buildTopQueriesQuery
participant PostgreSQL
Dashboard->>handleTopQueries: Request Top Queries with exclude_collector
handleTopQueries->>buildTopQueriesQuery: Build filters and bind arguments
buildTopQueriesQuery-->>handleTopQueries: SQL excluding probe and internal markers
handleTopQueries->>PostgreSQL: Query pg_stat_statements
PostgreSQL-->>handleTopQueries: Return filtered workload rows
handleTopQueries-->>Dashboard: Return Top Queries response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 425 |
| Duplication | 49 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
collector/src/probes/config_loader.go (1)
267-281: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSanitize the dynamic table identifier instead of raw
fmt.Sprintf.
lastCollectionTimeQueryinterpolatesprobeNamedirectly intoFROM metrics.%s, suppressed with#nosec G201. The sibling chokepoint instorage.go'sbuildMetricsInsertalready sanitizes dynamic identifiers viapgx.Identifier{...}.Sanitize()— applying the same pattern here removes the need for the suppression and keeps the two datastore-write/read chokepoints consistent.🛡️ Proposed fix
func lastCollectionTimeQuery(probeName string) string { + table := pgx.Identifier{"metrics", probeName}.Sanitize() return sqlmarker.Tag(fmt.Sprintf(` SELECT MAX(collected_at) - FROM metrics.%s + FROM %s WHERE connection_id = $1 - `, probeName)) + `, table)) }Based on learnings from a prior review of
pkg/datastoreconfig/datastoreconfig_test.go(PR 123) and this repo's established use ofpgx.Identifier{...}.Sanitize()for dynamic identifiers (as reinforced in the memory_tools_embedding_db_test.go learning): "For dynamic PostgreSQL identifiers (e.g., schema/table/column names) that cannot be parameterized, require using pgx.Identifier{identifier}.Sanitize() as the safe mechanism".🤖 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 `@collector/src/probes/config_loader.go` around lines 267 - 281, Update lastCollectionTimeQuery to sanitize probeName with the established pgx.Identifier{...}.Sanitize() pattern before interpolating it into the metrics table reference, matching storage.go’s buildMetricsInsert behavior. Remove the now-unnecessary `#nosec` G201 suppression and retain the existing query tagging and filtering logic.Source: Learnings
server/src/internal/api/perf_summary_handlers.go (1)
1065-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct, DB-independent unit test for
buildTopQueriesQuery.This is a pure function that owns the security-sensitive whitelist-dependent interpolation (
orderBy/order) and the new internal-marker exclusion clause, but it's currently only exercised indirectly throughtop_queries_handler_test.go, which skips entirely withoutTEST_AI_WORKBENCH_SERVERset. A pure unit test asserting on the returned SQL/args (e.g., placeholder numbering, presence/absence ofexcludeWorkbenchQueriesClause) would give fast CI coverage independent of a live datastore.🤖 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 `@server/src/internal/api/perf_summary_handlers.go` around lines 1065 - 1129, The pure query builder buildTopQueriesQuery lacks direct unit coverage for its SQL interpolation and optional clauses. Add DB-independent tests that call buildTopQueriesQuery and assert returned SQL and arguments, including placeholder numbering with and without queryID, correct inclusion or omission of excludeWorkbenchQueriesClause, and the interpolated order fields.
🤖 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 @.claude/golang-expert/internal-query-markers.md:
- Around line 1-9: Hide or remove the opening copyright preamble in
internal-query-markers.md so it does not render visibly; use an HTML comment if
retaining the copyright text, ensuring the document title remains the first
visible content.
In @.claude/golang-expert/partitioning.md:
- Around line 41-49: Narrow the marker-coverage documentation: in
.claude/golang-expert/partitioning.md lines 41-49, include partitionExistsQuery
among the partition-maintenance helpers or remove the claim that the listed
helpers build every statement; in docs/changelog.md lines 261-273, state that
the newly covered collector and alerter statements are individually tagged,
while server traffic and collector configuration resolution remain explicitly
untagged.
---
Nitpick comments:
In `@collector/src/probes/config_loader.go`:
- Around line 267-281: Update lastCollectionTimeQuery to sanitize probeName with
the established pgx.Identifier{...}.Sanitize() pattern before interpolating it
into the metrics table reference, matching storage.go’s buildMetricsInsert
behavior. Remove the now-unnecessary `#nosec` G201 suppression and retain the
existing query tagging and filtering logic.
In `@server/src/internal/api/perf_summary_handlers.go`:
- Around line 1065-1129: The pure query builder buildTopQueriesQuery lacks
direct unit coverage for its SQL interpolation and optional clauses. Add
DB-independent tests that call buildTopQueriesQuery and assert returned SQL and
arguments, including placeholder numbering with and without queryID, correct
inclusion or omission of excludeWorkbenchQueriesClause, and the interpolated
order fields.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 024d9bb1-54d3-4211-8b2a-4bb53a64258a
📒 Files selected for processing (25)
.claude/golang-expert/internal-query-markers.md.claude/golang-expert/partitioning.mdMakefilealerter/src/internal/database/metric_queries.goalerter/src/internal/database/queries.goalerter/src/internal/database/sql_marker_test.gocollector/src/database/datastore.gocollector/src/database/probe_availability.gocollector/src/database/probe_availability_test.gocollector/src/probes/change_tracking.gocollector/src/probes/config_loader.gocollector/src/probes/partition.gocollector/src/probes/sql_marker_integration_test.gocollector/src/probes/sql_marker_test.gocollector/src/probes/storage.gocollector/src/scheduler/scheduler.gocollector/src/scheduler/sql_marker_test.godocs/admin-guide/api/openapi.jsondocs/changelog.mdpkg/sqlmarker/sqlmarker.gopkg/sqlmarker/sqlmarker_test.goserver/src/internal/api/openapi.goserver/src/internal/api/perf_summary_handlers.goserver/src/internal/api/top_queries_filter_test.goserver/src/internal/api/top_queries_handler_test.go
…ifiers The two end-to-end tests asserting that the marker survives into pg_stat_statements failed on every CI matrix job, because CI's PostgreSQL containers install the extension but do not load it via shared_preload_libraries. Guarding on CREATE EXTENSION was not enough: that succeeds, and it is the subsequent read of the view that raises SQLSTATE 55000. Both tests now attempt the read and skip on failure, following the pattern pg_stat_statements_probe_test.go already used, now consolidated into one shared helper. Because those tests skip in CI, the alerter's tagging had nothing verifying it there: its existing test proved only that the registry SQL could be tagged, not that the code path tags it. The chokepoint now sits behind a one-method interface, and a database-free test drives every latestSQL and historicalSQL in the registry through it and asserts the statement text comes out tagged. Codacy flagged thirteen sites under its SQL-injection rule, and whilst most were false positives (fixed literals made non-constant by the marker comment, or concatenated bind arguments), three were worth acting on. createPartitionSQL and protectedPartitionsQuery interpolated relation names without quoting them, and partitionCandidatesQuery inlined the parent name into a string literal where a bind parameter belonged; identifiers are now quoted via pgx.Identifier and the parent name is bound as $1. The same unquoted interpolation in lastCollectionTimeQuery is fixed for consistency. The remaining sites carry the repository's existing nosemgrep annotation with a reason specific to each, and the #nosec G201 annotations that the earlier refactor displaced are restored on the builders that still need them. Also narrow the changelog and knowledge-base wording, which claimed more coverage than the change delivers: the server's own datastore traffic and the collector's probe_configs path remain untagged.
A security review of the exclusion clause turned up one real defect and three worthwhile tightenings, all in code this change already touches. The clause used a bare NOT LIKE against a nullable column, and NULL NOT LIKE '...' evaluates to NULL rather than true, so any pg_stat_statements row whose query text was not captured vanished from the panel whenever the toggle was on. That is backwards: a row we cannot positively identify as Workbench traffic should stay visible. The clause now admits a NULL query explicitly. On its own that would have been cosmetic, because the handler scanned the column into a string and dropped the row on the resulting scan error, so the scan now goes through a pointer and leaves the text empty instead. Extracting buildTopQueriesQuery left the ORDER BY column and direction interpolated a call frame away from the whitelist that makes them safe. The builder now re-checks both against that same whitelist and falls back to the defaults, so the property is local to the function doing the interpolation; the handler still rejects invalid input with a 400. Tag's behaviour on awkward input is now pinned by tests rather than left as folklore: dollar-quoted bodies are never entered, only the first statement of a batch is tagged, and a leading string-literal prefix such as E'x' would be corrupted by comment insertion. The last of those is unreachable, since no valid statement begins with a bare literal and every call site passes a leading keyword, and the doc comment now records why. Finally, the three #nosec G201 annotations move from function doc comments onto the lines they justify, because a doc-comment annotation suppresses the rule across the whole function body and would silently cover a future unsafe interpolation. Also document, in the Top Queries section of the server dashboard guide, that the toggle is a display convenience rather than an audit control, since a user who can run arbitrary SQL can include the marker text and evade it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The "Hide monitoring queries" toggle on the Server dashboard's Top
Queries panel filtered on the
ai_dba_wb_probemarker alone, which thecollector wraps only around the read-only probe queries it runs against
monitored databases. Because the metadata datastore normally shares a
PostgreSQL instance with those databases,
pg_stat_statementsalsocaptures the Workbench's own traffic against its own datastore, and
none of that carried a marker: the collector's bulk
metrics.*inserts, its partition maintenance and change-detection reads, and the
alerter's metric-evaluation queries all leaked through the filter.
(
ai_dba_wb_internal) from a new sharedpkg/sqlmarkerpackage,applied at the few chokepoints each binary funnels its datastore
traffic through rather than at the individual SQL literals, so that
statements added later are tagged by construction. The alerter needed
a single funnel (
queryInternal), through which all of the metricregistry's hundred-odd
latestSQL/historicalSQLliterals now pass.clause carrying no user input.
excluding the metadata database wholesale, because users legitimately
run other tools against that same data and expect to keep seeing them
in this panel.
Marker placement is load-bearing
Worth knowing before reviewing
sqlmarker.Tag, because the obvioustidy-looking placement is the one that does not work: PostgreSQL does
not preserve a comment in every position when it normalises a statement
for
pg_stat_statements. Measured on PostgreSQL 18:/* m */ INSERT INTO t ...INSERT /* m */ INTO t ...INSERT INTO t VALUES (1) /* m */;INSERT INTO t VALUES (1); -- mThe marker therefore sits immediately after the leading keyword, and
the rationale is documented on the helper so that nobody moves it to
the front of the string and silently breaks the filter. Relatedly,
queryidis derived from the parse tree and ignores comments, so atagged and an untagged form of the same statement share a
queryidandthe first-seen text wins; that is harmless for statements unique to the
Workbench, and is noted in the helper's doc comment.
Beyond the three categories named in the issue, the sweep also caught
the scheduler's per-cycle database-list query, which runs against the
monitored server every cycle and bypasses
WrapQueryentirely, so itwas unmarked noise the issue had not spotted.
Test plan
sqlmarker.Tag: each leading keyword,newline-indented raw literals, empty and whitespace-only input,
idempotency, and a case asserting the marker is not placed at the
start of the string, guarding the behaviour tabled above.
chokepoint, and that the server clause excludes both markers.
pg_stat_statements. These use throwaway tables deliberately: astatement differing only by a column alias or a literal collapses
onto the same
queryidand keeps the older text, so only a uniquelynamed relation can identify a test statement.
make test-all, plus the alerter target separately, andgolangci-lintclean across collector, server and alerter. Coveragemeets the 90% floor on touched code; notable movers are
handleTopQueries0 to 98.5%,UpsertProbeAvailability0 to 100%,and
alerter/internal/database83.6 to 89.7%.cd server && make openapiregenerated, OpenAPI tests pass.server/internal/tools(
TestStoreMemory.../TestRecallMemories...,expected 3 dimensions, not 4000). They reproduce on pristinemain, stem froma stale
vector(3)fixture, and are unrelated to this change.Follow-ups, deliberately not in scope
conversations, timeline) is the same class of noise and is still
untagged. Unlike the alerter there is no funnel: roughly 300 direct
pool.Query/Execcall sites across about 15 packages. Thetractable shape is a tagging wrapper trio on the server
Datastoreplus a mechanical sweep. The collector's
probe_configspath belongswith it.
pkgmodule's tests now run from the rootmake testandmake test-all, which previously ran them nowhere. No CI workflowcovers that module either, so wiring one up remains outstanding.
Closes #364
Summary by CodeRabbit