Add chlog CLI with chcat/chtail/chgrep entrypoints #1

Merged

benvin merged 2 commits from benvin/initial into main 2026-08-23 17:18:28 +10:00

Conversation 2 Commits 2 Files Changed 25

+2171 -1

unkin-agent commented 2026-08-23 16:43:37 +10:00

Member

Copy Link

Copy Source

Why

The ClickHouse log store (logs.raw, ~281M rows/day, 3-day TTL, no text index) is currently only queryable by hand-writing HTTP queries — easy to get wrong and easy to accidentally fire an unbounded scan that runs into the server's 120s kill. This adds a purpose-built CLI so reading, following and searching logs is one command, with the cost guardrails baked in.

What

• Single chlog binary with cat/tail/grep subcommands; chcat/chtail/chgrep installed as symlinks and dispatched on argv[0], each with its own shell completions
• Every query is time-bounded (default --since 1h) and fully parameterized via ClickHouse HTTP {name:Type} params — no user input ever interpolated into SQL
• chcat pages large ranges as bounded keyset queries with boundary-timestamp dedupe
• chtail follows with a 2s poll, overlap re-query and dedupe so late-arriving rows surface exactly once
• chgrep does substring/-i/--regex message search plus --fields k=v, and refuses unfiltered searches wider than 6h without --force
• Common filters: -n/--namespace, --host, --pod, --container, --app, --severity, --stream, --limit; output text (colored, no-TTY safe), json, logfmt
• Creds via CH_URL/CH_USER (default logreader)/CH_PASSWORD
• Packaging mirrors node-lookup: Makefile with patch/minor/major, nfpm RPM (binary + symlinks + bash/zsh/fish completions), woodpecker pre-commit/test/build PR workflows and a tag release pipeline publishing to artifactapi rpm-internal
• Unit tests cover the query builder (bounds + params + injection), time parsing, pager/tail dedupe against a mocked ClickHouse HTTP server, formatting and the grep guard; go test -race, vet and gofmt clean

## Why The ClickHouse log store (`logs.raw`, ~281M rows/day, 3-day TTL, no text index) is currently only queryable by hand-writing HTTP queries — easy to get wrong and easy to accidentally fire an unbounded scan that runs into the server's 120s kill. This adds a purpose-built CLI so reading, following and searching logs is one command, with the cost guardrails baked in. ## What - Single `chlog` binary with `cat`/`tail`/`grep` subcommands; `chcat`/`chtail`/`chgrep` installed as symlinks and dispatched on argv[0], each with its own shell completions - Every query is time-bounded (default `--since 1h`) and fully parameterized via ClickHouse HTTP `{name:Type}` params — no user input ever interpolated into SQL - `chcat` pages large ranges as bounded keyset queries with boundary-timestamp dedupe - `chtail` follows with a 2s poll, overlap re-query and dedupe so late-arriving rows surface exactly once - `chgrep` does substring/`-i`/`--regex` message search plus `--fields k=v`, and refuses unfiltered searches wider than 6h without `--force` - Common filters: `-n/--namespace`, `--host`, `--pod`, `--container`, `--app`, `--severity`, `--stream`, `--limit`; output `text` (colored, no-TTY safe), `json`, `logfmt` - Creds via `CH_URL`/`CH_USER` (default `logreader`)/`CH_PASSWORD` - Packaging mirrors node-lookup: Makefile with `patch`/`minor`/`major`, nfpm RPM (binary + symlinks + bash/zsh/fish completions), woodpecker `pre-commit`/`test`/`build` PR workflows and a tag release pipeline publishing to artifactapi `rpm-internal` - Unit tests cover the query builder (bounds + params + injection), time parsing, pager/tail dedupe against a mocked ClickHouse HTTP server, formatting and the grep guard; `go test -race`, vet and gofmt clean

unkin-agent added 1 commit 2026-08-23 16:43:37 +10:00

Add chlog CLI with chcat/chtail/chgrep entrypoints ... 415bf0cce1

Single Go binary for the ClickHouse log store (logs.raw): chlog with
cat/tail/grep subcommands, plus chcat/chtail/chgrep argv[0]-dispatched
symlink entrypoints. Every query is time-bounded and fully parameterized;
chgrep guards wide unfiltered scans. Ships nfpm RPM with completions and
woodpecker PR/tag pipelines mirroring node-lookup.

unkin-agent commented 2026-08-23 16:49:25 +10:00

Author

Member

Copy Link

Copy Source

VERDICT: ISSUES

Reviewed diff + body only, at head 415bf0c. Shallow-cloned benvin/initial, ran go test -race ./..., go vet ./..., gofmt -l . — all clean locally. One blocking issue found (CI's own lint gate), everything else checks out.

Blocking: ci/woodpecker/pr/test is red (lint step)

test.yaml's lint step runs golangci-lint run ./.... Reproduced locally with golangci-lint v2.13.1 (matching the golangci-lint:latest image tag the pipeline pulls) — default errcheck linter flags 6 unchecked error returns:

• internal/chlog/client.go:45 — io.WriteString(h, s) in Row.Key()
• internal/chlog/client.go:84 — defer resp.Body.Close()
• internal/chlog/client_test.go:58 — r.Body.Read(body)
• internal/chlog/client_test.go:83 — enc.Encode(row)
• main.go:144 — cmd.Flags().MarkHidden("until")
• main.go:145 — cmd.Flags().MarkHidden("limit")

None are functionally dangerous, but they're why the test job is failing (confirmed via commit-status: test = failure, build = success, pre-commit = pending). Please fix and let the pipeline go green.

Everything else looks solid

SQL injection safety — confirmed clean. Build() (internal/chlog/query.go) routes every user-controlled value (namespace/host/pod/container/app/stream/source/severity/pattern/--fields keys and values) through the Params map as {name:Type} placeholders — including field map keys, which is easy to get wrong and isn't here. Client.Run (client.go) sends the SQL as static body text and every value as a separate param_* HTTP form value, never string-built. TestBuildNoUserInputInSQL asserts injection payloads (incl. a DROP TABLE attempt and a fake {p:String} placeholder) never land in the SQL string.

Bounded-query discipline — Build() rejects missing/inverted since/until; --since defaults to 1h; chgrep's guard in newGrepCmd correctly blocks unfiltered windows >6h unless --force or Filter.Selective() (namespace/host/app) — tested for block/allow-filtered/allow-short-window. Page()'s keyset pagination + boundary dedupe is well tested, including a same-millisecond-burst edge case (TestPageSingleMillisecondBurst) that proves forward progress even when a page is saturated with same-timestamp rows. Tail()'s overlap+dedupe with a pruned seen-set is tested against concurrent late-arriving rows (TestTailDedupesAcrossPolls).

CLI conventions vs node-lookup — matches exactly: Makefile patch/minor/major tag-bump targets, nfpm RPM bundling the binary + 3 symlinks + bash/zsh/fish completions for all 4 entrypoint names, no-TTY-safe color (stdoutIsTTY() + NO_COLOR respected), 4-file woodpecker split (pre-commit/test/build on pull_request, release on tag) with backend_options.kubernetes.resources set on every step, tag release publishes the RPM to artifactapi rpm-internal. (serviceAccountName: default matches node-lookup's own existing convention — not a regression introduced here.)

Tests — meaningful, not rubber-stamped: query builder bounds/params/injection, time parsing (durations, RFC3339, invalid input), pager/tail dedupe against a mocked ClickHouse HTTP server, formatting (text/json/logfmt, color vs no-color), grep guard behavior. Good coverage of the exact edge cases called out in the PR description.

CI state: ci/woodpecker/pr/build success, ci/woodpecker/pr/test failure, ci/woodpecker/pr/pre-commit pending.

**VERDICT: ISSUES** Reviewed diff + body only, at head `415bf0c`. Shallow-cloned `benvin/initial`, ran `go test -race ./...`, `go vet ./...`, `gofmt -l .` — all clean locally. One blocking issue found (CI's own lint gate), everything else checks out. ## Blocking: `ci/woodpecker/pr/test` is red (lint step) `test.yaml`'s `lint` step runs `golangci-lint run ./...`. Reproduced locally with golangci-lint v2.13.1 (matching the `golangci-lint:latest` image tag the pipeline pulls) — default `errcheck` linter flags 6 unchecked error returns: - `internal/chlog/client.go:45` — `io.WriteString(h, s)` in `Row.Key()` - `internal/chlog/client.go:84` — `defer resp.Body.Close()` - `internal/chlog/client_test.go:58` — `r.Body.Read(body)` - `internal/chlog/client_test.go:83` — `enc.Encode(row)` - `main.go:144` — `cmd.Flags().MarkHidden("until")` - `main.go:145` — `cmd.Flags().MarkHidden("limit")` None are functionally dangerous, but they're why the `test` job is failing (confirmed via commit-status: `test` = failure, `build` = success, `pre-commit` = pending). Please fix and let the pipeline go green. ## Everything else looks solid **SQL injection safety** — confirmed clean. `Build()` (`internal/chlog/query.go`) routes every user-controlled value (namespace/host/pod/container/app/stream/source/severity/pattern/`--fields` keys *and* values) through the `Params` map as `{name:Type}` placeholders — including field map keys, which is easy to get wrong and isn't here. `Client.Run` (`client.go`) sends the SQL as static body text and every value as a separate `param_*` HTTP form value, never string-built. `TestBuildNoUserInputInSQL` asserts injection payloads (incl. a `DROP TABLE` attempt and a fake `{p:String}` placeholder) never land in the SQL string. **Bounded-query discipline** — `Build()` rejects missing/inverted since/until; `--since` defaults to `1h`; `chgrep`'s guard in `newGrepCmd` correctly blocks unfiltered windows >6h unless `--force` or `Filter.Selective()` (namespace/host/app) — tested for block/allow-filtered/allow-short-window. `Page()`'s keyset pagination + boundary dedupe is well tested, including a same-millisecond-burst edge case (`TestPageSingleMillisecondBurst`) that proves forward progress even when a page is saturated with same-timestamp rows. `Tail()`'s overlap+dedupe with a pruned seen-set is tested against concurrent late-arriving rows (`TestTailDedupesAcrossPolls`). **CLI conventions vs node-lookup** — matches exactly: Makefile `patch`/`minor`/`major` tag-bump targets, nfpm RPM bundling the binary + 3 symlinks + bash/zsh/fish completions for all 4 entrypoint names, no-TTY-safe color (`stdoutIsTTY()` + `NO_COLOR` respected), 4-file woodpecker split (`pre-commit`/`test`/`build` on `pull_request`, `release` on tag) with `backend_options.kubernetes.resources` set on every step, tag release publishes the RPM to artifactapi `rpm-internal`. (`serviceAccountName: default` matches node-lookup's own existing convention — not a regression introduced here.) **Tests** — meaningful, not rubber-stamped: query builder bounds/params/injection, time parsing (durations, RFC3339, invalid input), pager/tail dedupe against a mocked ClickHouse HTTP server, formatting (text/json/logfmt, color vs no-color), grep guard behavior. Good coverage of the exact edge cases called out in the PR description. **CI state**: `ci/woodpecker/pr/build` success, `ci/woodpecker/pr/test` failure, `ci/woodpecker/pr/pre-commit` pending.

unkinben added 1 commit 2026-08-23 16:49:35 +10:00

Fix errcheck lint failures in CI ... 4d78ed534b

golangci-lint errcheck flagged six unchecked error returns, failing the
pr/test workflow's lint step and skipping tests.

- Blank-assign hash writes in Row.Key and MarkHidden calls
- Close response body via deferred func matching node-lookup convention
- Read test request body with io.ReadAll instead of a single Body.Read

unkin-agent commented 2026-08-23 16:55:48 +10:00

Author

Member

Copy Link

Copy Source

VERDICT: CLEAN

Delta re-review of fix commit 4d78ed5 (on top of 415bf0c), scoped to its diff only against internal/chlog/client.go, internal/chlog/client_test.go, main.go.

All 6 errcheck findings from the prior review (comment 10483) are resolved, and each fix is genuinely cosmetic/unfailable — verified individually, not rubber-stamped:

• internal/chlog/client.go:45 — io.WriteString(h, s) in Row.Key() → _, _ = io.WriteString(...). h is fnv.New64a(); hash.Hash.Write is documented to never return an error. Safe to discard.
• internal/chlog/client.go:84 — defer resp.Body.Close() → defer func() { _ = resp.Body.Close() }(). Standard idiom for discarding a Close error on a response body already read to completion; matches the stated node-lookup convention.
• internal/chlog/client_test.go:58 — r.Body.Read(body) → body, _ := io.ReadAll(r.Body). This is actually a correctness improvement, not just a lint fix: the old single Read() call into a pre-sized buffer wasn't guaranteed to fill it. Ignoring the ReadAll error in a test fake is fine — a real failure here would surface as a body/SQL mismatch downstream in the same test.
• internal/chlog/client_test.go:83 — enc.Encode(row) → _ = enc.Encode(row). Test fake writing JSON to the response; an encode failure would show up as a client-side decode/assertion failure. Cosmetic.
• main.go:144-145 — cmd.Flags().MarkHidden("until") / MarkHidden("limit") → blank-assigned. Both flag names are hardcoded literals just registered via cf.register(cmd, 0) a few lines above, so MarkHidden's only error case (flag not found) is unreachable here.

No //nolint directives added anywhere — all findings genuinely fixed rather than suppressed. Diff is scoped exactly to the 6 flagged lines (git diff 415bf0c 4d78ed5 — 3 files, 7/-7 lines, nothing else touched).

Commit-status on 4d78ed5: ci/woodpecker/pr/build, ci/woodpecker/pr/test, ci/woodpecker/pr/pre-commit all success.

No action needed from my side beyond this confirmation — merge remains Ben's call.

**VERDICT: CLEAN** Delta re-review of fix commit 4d78ed5 (on top of 415bf0c), scoped to its diff only against `internal/chlog/client.go`, `internal/chlog/client_test.go`, `main.go`. All 6 errcheck findings from the prior review (comment 10483) are resolved, and each fix is genuinely cosmetic/unfailable — verified individually, not rubber-stamped: - `internal/chlog/client.go:45` — `io.WriteString(h, s)` in `Row.Key()` → `_, _ = io.WriteString(...)`. `h` is `fnv.New64a()`; `hash.Hash.Write` is documented to never return an error. Safe to discard. - `internal/chlog/client.go:84` — `defer resp.Body.Close()` → `defer func() { _ = resp.Body.Close() }()`. Standard idiom for discarding a Close error on a response body already read to completion; matches the stated node-lookup convention. - `internal/chlog/client_test.go:58` — `r.Body.Read(body)` → `body, _ := io.ReadAll(r.Body)`. This is actually a correctness improvement, not just a lint fix: the old single `Read()` call into a pre-sized buffer wasn't guaranteed to fill it. Ignoring the `ReadAll` error in a test fake is fine — a real failure here would surface as a body/SQL mismatch downstream in the same test. - `internal/chlog/client_test.go:83` — `enc.Encode(row)` → `_ = enc.Encode(row)`. Test fake writing JSON to the response; an encode failure would show up as a client-side decode/assertion failure. Cosmetic. - `main.go:144-145` — `cmd.Flags().MarkHidden("until")` / `MarkHidden("limit")` → blank-assigned. Both flag names are hardcoded literals just registered via `cf.register(cmd, 0)` a few lines above, so `MarkHidden`'s only error case (flag not found) is unreachable here. No `//nolint` directives added anywhere — all findings genuinely fixed rather than suppressed. Diff is scoped exactly to the 6 flagged lines (`git diff 415bf0c 4d78ed5` — 3 files, 7/-7 lines, nothing else touched). Commit-status on 4d78ed5: `ci/woodpecker/pr/build`, `ci/woodpecker/pr/test`, `ci/woodpecker/pr/pre-commit` all `success`. No action needed from my side beyond this confirmation — merge remains Ben's call.

benvin merged commit d09c671013 into main 2026-08-23 17:18:28 +10:00

benvin deleted branch benvin/initial 2026-08-23 17:18:28 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/clickhouse-tools#1