adaptive_export: restore dark-vector tracepoint table export onto main (8 dx_* tables) - #89
adaptive_export: restore dark-vector tracepoint table export onto main (8 dx_* tables)#89entlein wants to merge 45 commits into
Conversation
…le allowlist) Part A of aeprod28. Adds dx_execve, dx_vfs_events, dx_unlink, dx_dlookup, dx_mprotect, dx_creds, dx_bpf, dx_ptrace — the pid-keyed tracepoint tables for the dark vectors (V1/V2/V6/V7/V8). Generic 4-file allowlist edit (schema.sql, ddl.go KnownTables+PixieTables, apply.go OperatorOwnedTables, pxl/tables.go builtinTables) + the count guard. DDL is one-col/line (line-oriented verify parser); all carry the requiredPixieColumns (namespace/pod/hostname/time_) so VerifyPixieSchema passes. clickhouse + pxl tests green. TODO aeprod28: pid->pod pull enrichment, AE-owned no-TTL tracepoint deploy, pgsql-timeout (#7), steering rework (pixie-io#93/#62). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017k7uYSNUctQvkTAZYJbaB3
…tables (pixie-io#126) Part B+enrichment for aeprod28. #7 pgsql write: the firehose pull bounded query+write by cfg.Refresh (~30s) — far too tight for pgsql_events (full SQL text + heavy socket_tracer parse), so ExecuteScript hit context-deadline and pgsql landed 0 rows. Added a dedicated QueryTimeout (env-overridable, default 150s, matching the OrderQuery 180s budget); the pull now bounds the query by it, not Refresh. pixie-io#126 pid->pod enrichment: the 8 dark-vector tracepoint tables emit raw kernel pid (no upid) — the native px.upid_to_pod_name path fails on them. Added PodEnrichPxL: native tables keep upid resolution; dark tables merge process_stats on pid ONLY (the validated join-pod.pxl query — NOT pid+asid, since px.asid() is the kelvin asid on a dynamic tracepoint, not the per-PEM asid). Wired into both the passthrough (CompilePassthrough) and targeted (QueryFor) builders; the OrderQuery pod filter uses bare-pod equality for dark tables (their df.pod is the bare name). Test: TestPodEnrichPxL_DarkVsNative. Build + pxl/passthrough/clickhouse tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017k7uYSNUctQvkTAZYJbaB3
|
@ConstanzeTU (build-agent) — TTL image request 🏗️ Could you build a TTL adaptive_export image from this branch head (
Branch builds clean locally ( |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEight dark-vector tracepoint tables and three export presets are added. PxL enrichment now supports pid-keyed dark-vector data, while ClickHouse registration, plugin initialization, passthrough query timeouts, environment normalization, and mypy cache behavior are updated. ChangesDark-vector export support
Adaptive export runtime configuration
Tooling configuration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant AdaptiveExport
participant QueryFor
participant PodEnrichPxL
participant process_stats
participant ClickHouse
AdaptiveExport->>QueryFor: request dark-vector table data
QueryFor->>PodEnrichPxL: generate table-specific PxL
PodEnrichPxL->>process_stats: merge process metadata by pid
process_stats-->>PodEnrichPxL: return pod and namespace
PodEnrichPxL-->>QueryFor: return enriched PxL and pod filter
QueryFor->>ClickHouse: write rows to registered table
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/vizier/services/adaptive_export/internal/passthrough/passthrough.go`:
- Around line 73-80: Wire the new passthrough.Config.QueryTimeout field through
the production configuration path in cmd/main.go, including its environment or
configuration source and parsing alongside the existing passthrough settings.
Ensure the constructed Config passes the configured duration to the passthrough
service instead of relying on the hardcoded default.
- Around line 73-79: Align the QueryTimeout documentation with the value
assigned by New: either change New’s default from 150 seconds to the documented
180-second OrderQuery budget, or update the QueryTimeout comment to accurately
describe 150 seconds. Keep the comment and runtime default consistent.
In `@src/vizier/services/adaptive_export/internal/pxl/compile.go`:
- Around line 80-101: Update PodEnrichPxL to accept the query’s relStart value
and use it for the process_stats DataFrame instead of the hardcoded “-5m”
window. Pass the computed relStart from CompilePassthrough and QueryFor, and
adjust affected tests to cover historical query ranges.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 86fd4c39-1c08-4a70-b694-8ec5b64b1ac6
📒 Files selected for processing (9)
src/vizier/services/adaptive_export/internal/clickhouse/apply.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl.gosrc/vizier/services/adaptive_export/internal/clickhouse/schema.sqlsrc/vizier/services/adaptive_export/internal/passthrough/passthrough.gosrc/vizier/services/adaptive_export/internal/pxl/compile.gosrc/vizier/services/adaptive_export/internal/pxl/compile_test.gosrc/vizier/services/adaptive_export/internal/pxl/queryfor.gosrc/vizier/services/adaptive_export/internal/pxl/tables.gosrc/vizier/services/adaptive_export/internal/pxl/tables_test.go
| // QueryTimeout bounds a single table's pixie query (entlein/dx#7). The | ||
| // firehose pull used to bound query+write by Refresh, which is far too tight | ||
| // for a heavy protocol: pgsql_events carries full SQL text and its | ||
| // socket_tracer parse is expensive, so the ExecuteScript deadline-exceeded and | ||
| // pgsql_events landed 0 rows in forensic_db. Decoupled from Refresh and | ||
| // defaulted generous (matches the OrderQuery path's 180s budget). | ||
| QueryTimeout time.Duration |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the timeout documentation mismatch.
The comment says the default matches an OrderQuery 180-second budget, but New sets QueryTimeout to 150 seconds. Update either the default or the documentation so operators are not misled.
Also applies to: 116-118
🤖 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 `@src/vizier/services/adaptive_export/internal/passthrough/passthrough.go`
around lines 73 - 79, Align the QueryTimeout documentation with the value
assigned by New: either change New’s default from 150 seconds to the documented
180-second OrderQuery budget, or update the QueryTimeout comment to accurately
describe 150 seconds. Keep the comment and runtime default consistent.
| // QueryTimeout bounds a single table's pixie query (entlein/dx#7). The | ||
| // firehose pull used to bound query+write by Refresh, which is far too tight | ||
| // for a heavy protocol: pgsql_events carries full SQL text and its | ||
| // socket_tracer parse is expensive, so the ExecuteScript deadline-exceeded and | ||
| // pgsql_events landed 0 rows in forensic_db. Decoupled from Refresh and | ||
| // defaulted generous (matches the OrderQuery path's 180s budget). | ||
| QueryTimeout time.Duration | ||
| Tables []string |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Wire QueryTimeout into production configuration.
cmd/main.go constructs passthrough.Config without setting this field, so production always receives the hardcoded 150-second default. If this setting is intended to be configurable, add the corresponding environment/configuration wiring; otherwise, document that it is fixed.
🤖 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 `@src/vizier/services/adaptive_export/internal/passthrough/passthrough.go`
around lines 73 - 80, Wire the new passthrough.Config.QueryTimeout field through
the production configuration path in cmd/main.go, including its environment or
configuration source and parsing alongside the existing passthrough settings.
Ensure the constructed Config passes the configured duration to the passthrough
service instead of relying on the hardcoded default.
|
TTL adaptive_export image built ✅ — run 29839326430, built from PR head Pull either tag (same digest Only the AE image was rebuilt — kelvin/pem/metadata untouched. Also fixed the failing |
The custom bprm_execve kprobe does not fire on 6.x kernels (inlined/renamed); the shipped Pixie dc_snoop (kprobe:lookup_fast) is the working process+file instrument and captures live (MANIFESTO §10). Swap the dark-vector export: - tables.go: dx_execve -> dx_dcsnoop in builtinTables. - schema.sql / ddl.go / apply.go: dx_execve DDL -> dx_dcsnoop, + the dc_snoop `t` column (R=reference / M=miss from the dcache lookup). - compile.go: dx_dcsnoop is a pid-keyed dark-vector table (PodEnrichPxL merges process_stats on pid for pod+namespace). One dentry-cache probe serves both R0001 (process launch = binary lookup) and R0002 (file access). 14 AE packages green.
|
@ConstanzeTU (build-agent) — re-request TTL at new head Folded dc_snoop into the dark-table export and removed execve: Could you rebuild the TTL adaptive_export image from |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vizier/services/adaptive_export/internal/pxl/compile.go (1)
64-66: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftPass the query window into dark-vector enrichment.
PodEnrichPxL(table)still cannot use the caller’srelStart, soprocess_statsenrichment remains limited to the fixed five-minute window. Queries older than five minutes can therefore lose pod and namespace attribution. Accept and pass the computed query window, then add a regression test for a historical query.Also applies to: 80-102
🤖 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 `@src/vizier/services/adaptive_export/internal/pxl/compile.go` around lines 64 - 66, Update the compile flow around PodEnrichPxL to accept and forward the computed relStart and relEnd query window, and use those bounds when generating process_stats enrichment instead of the fixed five-minute range. Propagate the new parameters through the relevant enrichment helper while preserving existing output, and add a regression test covering a query older than five minutes with pod and namespace attribution.
🤖 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.
Outside diff comments:
In `@src/vizier/services/adaptive_export/internal/pxl/compile.go`:
- Around line 64-66: Update the compile flow around PodEnrichPxL to accept and
forward the computed relStart and relEnd query window, and use those bounds when
generating process_stats enrichment instead of the fixed five-minute range.
Propagate the new parameters through the relevant enrichment helper while
preserving existing output, and add a regression test covering a query older
than five minutes with pod and namespace attribution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1337e953-a2b1-414f-852d-d79224cc5313
📒 Files selected for processing (7)
src/vizier/services/adaptive_export/internal/clickhouse/apply.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl.gosrc/vizier/services/adaptive_export/internal/clickhouse/schema.sqlsrc/vizier/services/adaptive_export/internal/pxl/compile.gosrc/vizier/services/adaptive_export/internal/pxl/compile_test.gosrc/vizier/services/adaptive_export/internal/pxl/tables.gosrc/vizier/services/adaptive_export/internal/pxl/tables_test.go
|
TTL adaptive_export image rebuilt ✅ (dc_snoop fold, execve dropped) — run 29848515911, built from new head Pull either tag (same digest AE-only rebuild, |
proc.ctx['pod'] yields the NAMESPACED pod name (ns/pod) on Pixie v0.14.20+, not the bare pod name the ported comment assumed. Verified live (rig 6a5f6bc0): df.pod=='specimen/activity-gen-xxx' matches 2 rows, df.pod=='activity-gen-xxx' matches 0 — so every dark-vector pull silently returned empty while the native protocol pulls (namespaced key) worked. Match the namespaced key for dark vectors too. Native protocol export already proven end-to-end (dns/conn land in forensic_db for the steered pod).
|
@ConstanzeTU one more TTL at |
|
TTL adaptive_export image rebuilt ✅ (dark-vector pod filter fix: Pull either tag (same digest AE-only, |
…tion scripts at boot
The operator already reconciles ClickHouse-plugin retention scripts on boot
(installPresetScripts, gated INSTALL_PRESET_SCRIPTS=true: GetClusterScripts →
purge operator-managed → AddDataRetentionScript for each builtin). Add three
dark-vector/profiler export scripts to the builtin set so they are registered
IF-NOT-PRESENT, permanently, via the native OTel→ClickHouse plugin — no external
wrapper:
- ch-dc_snoop — UpsertTracepoint(lookup_fast, "876000h" ≈ permanent) + px.export
(dentry cache = process+file, V1/V2). bare bprm_execve/d_lookup
don't fire on 6.x (MANIFESTO §10); lookup_fast does.
- ch-stack_trace — native continuous profiler stack_traces.beta (V9, no tracepoint)
→ px.export. The OTel "profiles" stack-trace signal.
- ch-creds_change — UpsertTracepoint(commit_creds, "876000h") + px.export: a process
committing new uid==0 while its previous real uid>0 = privilege
escalation to root (V7).
Scripts embedded (go:embed internal/script/presets/*.pxl); each uses
px.plugin.start_time/end_time (plugin-managed window) + px.otel.ClickHouseRows.
Tracepoints use a ~100y TTL (no built-in "permanent" flag; a huge TTL is
effectively permanent + survives a cron stall). CH DDL for the 3 tables added to
schema.sql so the operator self-creates them. 14 AE packages green.
|
@ConstanzeTU TTL at |
|
TTL adaptive_export image built ✅ (3 boot-registered retention/export scripts: ch-dc_snoop, ch-stack_trace, ch-creds_change) — run 29860622585, from head Pull either tag (same digest AE-only, |
…ST_STREAM PROTOCOL_ERROR) A pixie-api-key sourced via a k8s secret created with `kubectl --from-file` keeps the file's trailing newline. Sent as the pixie-api-key gRPC metadata header to the cloud PluginService, that newline is an HTTP/2 header protocol violation → the server replies with RST_STREAM PROTOCOL_ERROR. It surfaces as "could not ensure ClickHouse plugin is enabled" / "get cluster scripts: ... PROTOCOL_ERROR" and blocks retention-script registration entirely, even though the key is valid (verified: same key via $(...) — which strips the newline — lists the scripts fine). TrimSpace the API key (and defensively the endpoint host, ClickHouse DSN, and cluster id) so a whitespace-padded secret can't break the cloud calls. Proven live: with the newline stripped the operator registers all 14 preset scripts (incl. ch-dc_snoop / ch-stack_trace / ch-creds_change) on the cluster.
|
@ConstanzeTU — ready for linter fix + release �my end is validated 🎯 Root-caused and fixed the retention-script registration failure: the pixie-api-key had a trailing newline (from a Two asks:
Then I'll deploy aeprod35 on a fresh rig and confirm end-to-end (scripts register + tables export). Thanks! |
The arc `mypy` linter (mypy 1.20.2, `show_column_numbers = True`) crashes with an INTERNAL ERROR on stack_trace.pxl in CI. The file was the only preset carrying a multibyte character the linter had not seen before — a `->` (U+2192 arrow) in a comment; no other .pxl in the tree uses one. With column reporting on, mypy's byte-vs-character column bookkeeping over a multibyte comment char is a known crash vector. The sibling presets that lint clean are ASCII (dc_snoop) or carry only an em-dash that does not trigger it (creds_change), so this narrows stack_trace.pxl's comments to ASCII to match the known-good profile. No script behaviour changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/vizier/services/adaptive_export/internal/config/config.go`:
- Around line 166-174: Update the debug logging in the configuration-loading
flow to stop emitting credential-bearing values, especially pixieAPIKey and
clickhouseDSN. Log only presence/status for the API key and a sanitized
connection description for the DSN, while preserving non-sensitive configuration
logging and behavior in the surrounding config loader.
- Line 174: Update the PIXIE_ENDPOINT handling around getEnvWithDefault and
pixieHost so whitespace-only values receive defPixieHostname after trimming,
allowing the existing Kubernetes fallback check to run. Apply trimming before
the defaulting decision or within getEnvWithDefault, and add a regression test
covering a whitespace-only PIXIE_ENDPOINT.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: f7b7804e-f8aa-4510-9b8c-46478995a9ae
📒 Files selected for processing (2)
src/vizier/services/adaptive_export/internal/config/config.gosrc/vizier/services/adaptive_export/internal/script/presets/stack_trace.pxl
| clickhouseDSN := strings.TrimSpace(os.Getenv(envClickHouseDSN)) | ||
| pixieClusterID := strings.TrimSpace(os.Getenv(envPixieClusterID)) | ||
| // TrimSpace: a secret sourced via `kubectl --from-file` keeps the file's | ||
| // trailing newline. In the pixie-api-key gRPC metadata header that newline is | ||
| // an HTTP/2 protocol violation → the cloud PluginService replies RST_STREAM | ||
| // PROTOCOL_ERROR (looks like an auth failure but isn't). Trim it defensively. | ||
| pixieAPIKey := strings.TrimSpace(os.Getenv(envPixieAPIKey)) | ||
| clusterName := strings.TrimSpace(os.Getenv(envClusterName)) | ||
| pixieHost := strings.TrimSpace(getEnvWithDefault(envPixieEndpoint, defPixieHostname)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the API key or credential-bearing DSN.
These values are emitted verbatim by the debug logs below, including pixieAPIKey and clickhouseDSN. Redact credentials and log only presence/status or sanitized connection details.
🤖 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 `@src/vizier/services/adaptive_export/internal/config/config.go` around lines
166 - 174, Update the debug logging in the configuration-loading flow to stop
emitting credential-bearing values, especially pixieAPIKey and clickhouseDSN.
Log only presence/status for the API key and a sanitized connection description
for the DSN, while preserving non-sensitive configuration logging and behavior
in the surrounding config loader.
| // PROTOCOL_ERROR (looks like an auth failure but isn't). Trim it defensively. | ||
| pixieAPIKey := strings.TrimSpace(os.Getenv(envPixieAPIKey)) | ||
| clusterName := strings.TrimSpace(os.Getenv(envClusterName)) | ||
| pixieHost := strings.TrimSpace(getEnvWithDefault(envPixieEndpoint, defPixieHostname)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A8 -B4 'func getEnvWithDefault|pixieHost :=|envPixieEndpoint' \
src/vizier/services/adaptive_export/internal/config/config.goRepository: k8sstormcenter/pixie
Length of output: 1780
🏁 Script executed:
#!/bin/bash
sed -n '160,240p' src/vizier/services/adaptive_export/internal/config/config.go
printf '\n---\n'
sed -n '330,350p' src/vizier/services/adaptive_export/internal/config/config.go
printf '\n---\n'
rg -n 'defPixieHostname|pixieHost|Kubernetes|fallback|work.pixie' src/vizier/services/adaptive_export/internal/config/config.goRepository: k8sstormcenter/pixie
Length of output: 4941
Apply the default after trimming
PIXIE_ENDPOINT values that are only whitespace become "" after TrimSpace, so the later pixieHost == defPixieHostname check skips the Kubernetes fallback. Trim before defaulting, or trim inside getEnvWithDefault, and add a regression test.
🤖 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 `@src/vizier/services/adaptive_export/internal/config/config.go` at line 174,
Update the PIXIE_ENDPOINT handling around getEnvWithDefault and pixieHost so
whitespace-only values receive defPixieHostname after trimming, allowing the
existing Kubernetes fallback check to run. Apply trimming before the defaulting
decision or within getEnvWithDefault, and add a regression test covering a
whitespace-only PIXIE_ENDPOINT.
arc lint runs `mypy --config-file=mypy.ini <file>` once per file, in parallel. The three adaptive_export preset .pxl files (added together) are linted concurrently and share the incremental cache in the repo root; concurrent writers corrupt it, producing a nondeterministic mypy `INTERNAL ERROR` on whichever file loses the race (stack_trace.pxl on one run, dc_snoop.pxl on the next -- neither a content issue). Setting `cache_dir = /dev/null` makes each per-file invocation self-contained, so there is no shared cache to race on. Type-checking semantics are unchanged; per-file runs get no incremental benefit anyway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Once the mypy cache crash was out of the way, arc lint surfaced two real violations on the three preset .pxl files: - missing Apache license header (all three) -- added the canonical header used by the other pxl scripts; - flake8 E127 (continuation line over-indented) on dc_snoop's pxtrace.UpsertTracepoint call -- collapsed to a single line, matching creds_change's call style. flake8 (.pxl.flake8rc) and mypy both clean locally on all three. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
CI linter green + release cut ✅ Linter — the red
All 8 PR checks green at Release — cut
|
|
Release
Cut from |
…zier crash) The retention plugin's export sink is the query engine's native ClickHouseExportSink (clickhouse-cpp over TCP :9000), not the AE's own HTTP write path (:8123). It requires the DSN in native format clickhouse://user:pass@host:9000/db. Passing the AE's HTTP DSN (http://host:8123/db) made the sink parse "http" as the username and crash on connect, taking the whole vizier Unhealthy. - config: add NativeDSN() builder (native TCP port, no http scheme), distinct from DSN() which remains the AE's own HTTP write endpoint. - main: pass NativeDSN() to EnsureClickHousePluginEnabled, and retry the ensure up to 5x (the vizier plugin service can 404 for the first few seconds after boot, permanently skipping enablement otherwise).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/vizier/services/adaptive_export/cmd/main.go`:
- Around line 267-274: Update the retry handling around
EnsureClickHousePluginEnabled so the warning log and six-second sleep execute
only when attempt < 5; preserve the existing retry behavior and final error
handling after the fifth failed attempt.
- Around line 261-268: Update the success logging in the retry flow around
EnsureClickHousePluginEnabled so it never emits the credential-bearing exportURL
returned from NativeDSN(). Log only plugin enabled/status information, or use a
properly redacted DSN if the endpoint must be included; preserve the existing
retry and enablement behavior.
In `@src/vizier/services/adaptive_export/internal/config/config.go`:
- Line 473: Format the clickhouse Host method and the surrounding config.go file
using the repository’s standard Go formatter or lint target, ensuring the
formatting error reported at Host is resolved without changing behavior.
- Around line 470-471: Update clickhouse.NativeDSN to construct the URL with
url.UserPassword for escaping credentials and net.JoinHostPort for correctly
formatting the host and port, while preserving the database component. Add a
regression test covering user and password values containing reserved characters
such as @, :, /, and %.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 85fe279c-2286-4218-a1e0-8d4ce4d3bcac
📒 Files selected for processing (6)
mypy.inisrc/vizier/services/adaptive_export/cmd/main.gosrc/vizier/services/adaptive_export/internal/config/config.gosrc/vizier/services/adaptive_export/internal/script/presets/creds_change.pxlsrc/vizier/services/adaptive_export/internal/script/presets/dc_snoop.pxlsrc/vizier/services/adaptive_export/internal/script/presets/stack_trace.pxl
| for attempt := 1; attempt <= 5; attempt++ { | ||
| exportURL, err = pluginClient.EnsureClickHousePluginEnabled(chDSN) | ||
| if err == nil { | ||
| break | ||
| } | ||
| log.WithError(err).WithField("attempt", attempt).Warn("ensure ClickHouse plugin enabled failed — retrying (vizier plugin service may still be starting)") | ||
| time.Sleep(6 * time.Second) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid sleeping after the final failed attempt.
The loop sleeps unconditionally after attempt five, adding an unnecessary six-second startup delay and logging “retrying” when no retry remains. Gate the warning and sleep on attempt < 5.
🤖 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 `@src/vizier/services/adaptive_export/cmd/main.go` around lines 267 - 274,
Update the retry handling around EnsureClickHousePluginEnabled so the warning
log and six-second sleep execute only when attempt < 5; preserve the existing
retry behavior and final error handling after the fifth failed attempt.
| func (c *clickhouse) NativeDSN() string { | ||
| return fmt.Sprintf("clickhouse://%s:%s@%s:%s/%s", c.user, c.password, c.host, c.port, c.database) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and locate NativeDSN usage.
git ls-files src/vizier/services/adaptive_export/internal/config/config.go
ast-grep outline src/vizier/services/adaptive_export/internal/config/config.go --view expanded || true
echo "---- usages ----"
rg -n "NativeDSN\(|clickhouse://%s:%s@|UserPassword|JoinHostPort|url\.URL" src/vizier/services/adaptive_export/internal -S || true
echo "---- nearby source ----"
sed -n '430,520p' src/vizier/services/adaptive_export/internal/config/config.goRepository: k8sstormcenter/pixie
Length of output: 8602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether any tests or callers already account for URL escaping.
rg -n "adaptive_export|NativeDSN|clickhouse" src -S || trueRepository: k8sstormcenter/pixie
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/vizier/services/adaptive_export/internal/config/config.goRepository: k8sstormcenter/pixie
Length of output: 3775
🌐 Web query:
ClickHouse Go DSN user password percent-encoding reserved characters @ : / % url.UserPassword
💡 Result:
When using a Data Source Name (DSN) string to connect to ClickHouse in Go, any special characters in the username or password—such as @,:, /,?, or +—must be percent-encoded to prevent the DSN from being parsed incorrectly [1][2][3][4]. If you are constructing the DSN string manually, you should encode the username and password components using url.QueryEscape before inserting them into the connection string [3][5]. Failure to do so often results in "Authentication failed" errors (Code: 516) or syntax/parsing errors because the driver or URL parser misinterprets special characters as structural separators (e.g., @ acting as a host delimiter or: as a password separator) [1][3][4]. Alternatively, you can avoid manual DSN construction and potential encoding issues by using the clickhouse.Options struct provided by the clickhouse-go driver [6][7]. This approach allows you to pass the username and password as standard string fields in the Auth configuration, bypassing the need for manual URL encoding [6][7]: go conn, err := clickhouse.Open(&clickhouse.Options{ Addr: []string{"localhost:9000"}, Auth: clickhouse.Auth{ Database: "default", Username: "your_username", Password: "your_password_with_special_chars@!#", }, }) If you must use url.UserPassword to generate user info for a URL object, note that Go's net/url package automatically applies appropriate encoding for the userinfo component, which escapes characters like @, /,?, and: [5][8][9][10]. However, using the clickhouse.Options struct is the recommended best practice for configuration in the ClickHouse Go client to ensure credentials are handled securely and correctly [1][2][6].
Citations:
- 1: https://clickhouse.com/docs/integrations/language-clients/go/config-reference
- 2: https://github.com/ClickHouse/clickhouse-docs/blob/main/docs/integrations/language-clients/go/config-reference.md
- 3: Authentication failed if password contains specific symbols ClickHouse/clickhouse-go#362
- 4: bug: ClickHouse migration fails when password contains special characters - needs URL encoding in up.sh langfuse/langfuse#12077
- 5: https://stackoverflow.com/questions/48671938/go-url-parsestring-fails-with-certain-user-names-or-passwords
- 6: https://clickhouse.com/docs/integrations/go
- 7: https://github.com/ClickHouse/clickhouse-go/blob/main/README.md
- 8: https://go.dev/src/net/url/gen_encoding_table.go?m=text
- 9: https://go.dev/src/net/url/encoding_table.go
- 10: proposal: net/url: adhere to RFC3986 golang/go#16127
Escape credentials in NativeDSN. Raw user/password values can break the DSN when they contain reserved characters like @, :, /, or %. Build it with url.UserPassword and net.JoinHostPort, and add a regression test for special-character credentials.
🤖 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 `@src/vizier/services/adaptive_export/internal/config/config.go` around lines
470 - 471, Update clickhouse.NativeDSN to construct the URL with
url.UserPassword for escaping credentials and net.JoinHostPort for correctly
formatting the host and port, while preserving the database component. Add a
regression test covering user and password values containing reserved characters
such as @, :, /, and %.
Source: MCP tools
The retention plugin's ClickHouseExportSink (clickhouse_export_sink_node.cc) serializes Pixie columns with a fixed type mapping and auto-appends an event_time column as DateTime64(3) (milliseconds). The dark-vector tables were declared with types that don't match what the sink sends, so every INSERT threw TYPE_MISMATCH server-side and the clickhouse-cpp client segfaulted on ReceiveException — crashlooping kelvin. Corrected to the sink's actual output types: - time_ UInt64 -> DateTime64(9) (TIME64NS) - upid UInt128 -> String (UINT128 serialized as String) - pid Int32 -> Int64 (all Pixie ints are INT64) - old/new_uid UInt32 -> Int64 - event_time DateTime64(9) -> DateTime64(3) (sink auto-appends millis) Validated live: stack_trace exports 5609+ rows of real profiler data, 0 insert errors, kelvin stable, vizier Healthy. NOTE (follow-up): the protocol builtinPresetScripts tables (http_events/dns_events/conn_stats/pgsql_events + redis/mysql/cql/mongodb/ amqp/mux/tls_events) export through the same sink and have the identical event_time DateTime64(3) requirement, currently unmet — latent until those tables receive rows.
…me64(9) The ClickHouse export sink auto-appends an event_time column as DateTime64(3) (milliseconds) ONLY when the exported df has a time_ column but no event_time column (clickhouse_export_sink_node.cc:186 `has_time_ && !has_event_time`). That millisecond column mismatches the tables' DateTime64(9) event_time and crashes the native client on INSERT. Rather than degrade every table to DateTime64(3) millis (which would break the nanosecond-consistent event_time contract shared with the AE HTTP write path and dx/soc joins — see the schema.sql header), each preset now sets `df.event_time = df.time_`. That makes the sink treat event_time as a normal TIME64NS column and emit DateTime64(9) nanoseconds, matching the schema. - dc_snoop.pxl / creds_change.pxl / stack_trace.pxl: add df.event_time = df.time_ - builtinPresetScripts (all protocol presets): same, before px.display - schema.sql: dark-table event_time reverted DateTime64(3) -> DateTime64(9) (the other sink-type fixes stay: time_ DateTime64(9), upid String, pid/uid Int64)
…s + traces) Retention/cron export scripts cannot deploy a tracepoint — the cron executor drops the pxtrace mutation, so dc_snoop/creds_change tables were never created (Table not found forever), while stack_trace worked only because it's the native profiler (no tracepoint). The AE now owns tracepoint deployment. - script.DesiredTracepoints(): source of truth for the bpftraces the AE deploys (dc_snoop, creds_change; extend for V6/V8). Each has a <name>_deploy.pxl (import pxtrace + UpsertTracepoint, permanent TTL, idempotent upsert). - main.deployDesiredTracepoints: at boot (INSTALL_PRESET_SCRIPTS=true) run each deploy script as a mutation ExecuteScript over the pixie adapter, with retry. pxapi auto-sets Mutation:true for `import pxtrace`. - Split the export presets: dc_snoop.pxl / creds_change.pxl are now query+export ONLY (no UpsertTracepoint) — they read the already-deployed table. - CONTRACTS.md: C12 expanded to "AE owns schemata + table deployments + trace deployments"; new C16 (native-DSN export + nanos event_time) and C17 (AE deploys bpftraces, cron never does); boot diagram updated.
A compiled ARM aarch64 ELF executable named `cmd` (91MB) was committed at the repo root -- accidental `go build`/bazel output, not gitignored. It bloats the tree and would trip filename/artifact lint. Removed; no source or BUILD change (the real binary target is //src/vizier/services/adaptive_export/cmd). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
TTL adaptive_export image built ✅ (wire EXPORT_MODE=never to disable trigger self-steer) — run 30536809219, from head Pull either tag (same digest
Say the word for annotated |
golangci-lint's gofumpt formatter flagged presets.go:16 -- gofumpt wants a blank line between the two consecutive top-level var blocks (defaultExcludeNamespaces / defaultExcludeComms). Applied gofumpt -w to that file only. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iltin golangci flagged main.go: (1) gci/gofumpt formatting — the new env-var consts and controller.Config fields broke alignment; (2) predeclared — leaderNode's local var `min` shadows the Go 1.21 builtin. Reformatted with gci+gofumpt (repo sections standard/default/prefix(px.dev)) and renamed the var to `smallest`. Builds clean; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two CI lint failures from the config-driven-presets work: - internal/script/BUILD.bazel: presets_test.go was added without the gazelle-generated pl_go_test target -> 'Gazelle was not run'. Added the script_test target (matches gazelle diff + cmd/BUILD.bazel style). - dc_snoop.pxl: the runtime sentinel '#__DC_SNOOP_EXCLUSION__' tripped flake8 E265 (block comment needs '# '). Renamed to '# __DC_SNOOP_EXCLUSION__' and updated the matching strings.Replace target in presets.go so the env-injected exclusion still substitutes byte-for-byte. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
TTL image from green tip + full CI lint now passing ✅ — image build 30541746788, head Pull either tag (same digest All 9 checks green (
Only thing still blocking merge is the CodeRabbit review (CHANGES_REQUESTED — the credential-logging thread on config.go/main.go), not CI. Say the word for annotated |
…nce lands The dark tables (dc_snoop/creds_change/dx_*) came back EMPTY: the AE filtered them by pod, but an incident's transient malignant pids (whoami/cat/getent children) are too short-lived to enter process_stats, so their ns/pod resolves blank and the filter dropped exactly the evidence. The AE is node-local (pem-direct → the node's own PEM), so QueryFor now keeps every dark row in the window and only drops the infra/self comms (DC_SNOOP_EXCLUDE_COMMS, env-tunable) — the workload's dark activity is captured node-scoped, no relevance filtering.
|
TTL adaptive_export image built ✅ (node-scope dark-vector tables so the malignant evidence lands) — run 30544817371, from head Pull either tag (same digest AE-only, |
pem-direct is the robust query path for the node-scoped AE: node-local (matches its scope), desync-immune (bypasses the kelvin/broker aggregation that the recurring PEM desync silently breaks) and fast. Default to HOST_IP:50305 when the deploy provides HOST_IP (downward API) + PL_JWT_SIGNING_KEY; explicit ADAPTIVE_VIZIER_DIRECT_ADDR still wins; cloud passthrough only as fallback.
|
@build-agent — please cut a TTL image of the current head
TTL tag is fine; I'll deploy it on a fresh pixie-test rig. Thanks! |
The stray adaptive_export ELF binary got committed at the repo root again (a repo-root `go build` output). Removed it and added `/cmd` to .gitignore so it stops recurring. No source/BUILD change; the real binary target is //src/vizier/services/adaptive_export/cmd. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@build-agent thanks for the |
|
TTL adaptive_export image built ✅ (default to pem-direct, node-local PEM :50305) — run 30546948463, from head Pull either tag (same digest AE-only, 🧹 The 91MB |
Follow-up to fd9108a, which removed the binary but did not land the .gitignore rule. Adds `/cmd` so a repo-root `go build` output stops getting re-committed. No source/BUILD change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Already up — the |
…insert=0 writes Two fresh-rig blockers for the full evidence set: 1. pem-direct (:50305) serves fast node-local queries but REFUSES mutations (direct-query: mutations out of scope #29), so the bpftrace deploy failed and the dark tables (dc_snoop/creds_change/stack_trace) stayed empty. Deploy the tracepoints via the in-cluster broker (:50300) with the same JWT when the query adapter is pem-direct; keep querying via pem-direct. 2. Fresh ClickHouse defaults async_insert=1 → the AE's INSERT buffered and returned written_rows=0 (evidence trickled in minutes later / looked lost). Write with SETTINGS async_insert=0 so evidence lands + is counted immediately.
|
@build-agent one more TTL please — head
|
|
TTL adaptive_export image built ✅ (tracepoints via broker-direct on pem-direct + async_insert=0 evidence writes) — run 30549801480, from head Pull either tag (same digest AE-only, |
| ) ENGINE = MergeTree ORDER BY (event_time, pod); | ||
|
|
||
| -- stack_trace (native continuous profiler stack_traces.beta, V9) — OTel export. | ||
| CREATE TABLE IF NOT EXISTS forensic_db.stack_trace ( |
There was a problem hiding this comment.
why is this not the pixie native table? dont reinvent things, please
|
@build-agent TTL please — head |
|
TTL adaptive_export image built ✅ (stabilize dc_snoop: comm-filter before the process_stats merge) — run 30556800261, from head Pull either tag (same digest AE-only, |
|
@build-agent TTL please — head |
|
TTL adaptive_export image built ✅ (control: read steer timestamps as nanoseconds — root-cause fix for dark-tables-empty on the dx path) — run 30564781605, from head Pull either tag (same digest AE-only, |
…ess_stats merge) The dc_snoop node-scope query timed out / silently dropped: it merged the node's ENTIRE dark stream (Formatter/vector/runc/... thousands of rows/window) against process_stats before filtering. Reorder: drop the infra/self comms FIRST, then merge — the pid-join now runs on the handful of workload rows so the dark capture completes reliably. Also expand the default comm-exclusion (runc:[2:INIT], CgrpMemUsgObsr, Formatter, iptables-save, vector-worker, metrics-server, ...) in CODE, since the env value with '[' ':' breaks the PxL filter.
…ne unit) The control API (dx -> AE: /export/start t_end, /query window) carries unix timestamps, and the evidence pipeline's ONE unit is nanoseconds (event_time, dx referral windows). The handlers read them with time.Unix(v, 0) — i.e. as SECONDS — so dx's nanosecond t_end (~1.78e18) became a year-56-billion instant. Effect: every dx-steered OrderExportAll captured [t_end-600s, t_end] over that garbage instant, a window that overlaps NO data, so the full-evidence set (all dark tables: dc_snoop/conn_stats/creds_change/stack_trace, and every protocol table) silently returned zero rows on the dx-steered path. Only the self-steer path (its own now-based window) ever wrote anything, which is why redis_events populated but the dx-ordered dark capture never did. The same garbage window also flowed into ae_reconcile.win_start/win_end, overflowing the DateTime formatter and making CH reject the reconcile insert (HTTP 400) — which hid the whole failure from the reconcile instrument. Fix: read the control timestamps as nanoseconds, time.Unix(0, ns), matching the pipeline. No unit autodetection — one unit, everywhere. Test + struct doc-comments updated to nanoseconds.
1adc791 to
325160f
Compare
The dark-vector pod-resolution merge scanned process_stats over a 5-minute window. On a busy node that table samples every live pid every ~10-30s, so the scan is large and is the dominant cost of the dark query — heavy enough that, sharing the fan-out's query-slot budget with the fast native-table queries, the dark capture either starved (too few slots) or, once the steer windows were real, saturated the node-local PEM (too many). A 2-minute window still resolves the pods that matter — long-lived workload pids (redis-server) are sampled continuously — while transient attack pids never enter process_stats and resolve blank either way. Cuts the merge scan ~2.5x so the dark tables complete within the fan-out budget alongside the native queries.
|
@build-agent TTL please — head |
|
TTL adaptive_export image built ✅ (rebased head) — run 30566996632, from head Pull either tag (same digest Note: this was a rebase of the last two commits ( |
|
TTL adaptive_export image built ✅ (bound dark-table process_stats scan 5m→2m) — run 30567580670, from head Pull either tag (same digest AE-only, |
2 participants
Summary: Restore the adaptive_export dark-vector tracepoint table export onto main. The dark-table export was built on
feat/ae-dark-tables-pgsql-steering(ec8382588+ pid→pod enrichmentddf6d18e3) and never merged, so main and everyaeprod29→aeprod34/v0.14.20build carry only the 13 protocol tables. This cherry-picks the two dark-table commits cleanly onto main: 8dx_*tables (dx_execve,dx_vfs_events,dx_unlink,dx_dlookup,dx_mprotect,dx_creds,dx_bpf,dx_ptrace) with their ClickHouse DDL, plusIsDarkVector+PodEnrichPxLpid-keyed enrichment (dark tables have noupid, so pod is resolved via aprocess_statspid-merge instead ofupid_to_pod_name), and the pgsql passthrough query timeout from the same commit. Net export set becomes 8 dark + 13 protocol = 21 tables; the 6 control/evidence tables are unchanged.Test Plan:
go build ./src/vizier/services/adaptive_export/...passes;go test ./src/vizier/services/adaptive_export/...is green across 14 packages with 0 failures; theschema.sqlembed is already covered by the existingembedsrcson main so no BUILD.bazel change is needed. Live validation against deployed tracepoints on a v0.14.20 rig follows on the TTL adaptive_export image before the release build is cut.Type of change: /kind feature
Changelog Message: adaptive_export now exports the 8 dark-vector tracepoint tables (
dx_*) alongside the 13 protocol tables, with pid-keyed pod enrichment for the dark tables.Full context (Why / What / Validation)
Why
The AE dark-vector tracepoint table export was built on
feat/ae-dark-tables-pgsql-steering(commitec8382588+ pid→pod enrichmentddf6d18e3) and never merged to main.merge-base --is-ancestorconfirms it: main'sinternal/pxl/tables.gohas 0dx_*tables. Theaeprod29→aeprod34line (and the currentv0.14.20build) were all cut from the main line and carry only the 13 protocol tables — the dark tables were only ever in the one-offaeprod28-rc1image.The dx generic-precorrelation loop needs AE to export the full evidence slice — all dark tables + all protocols per kubescape anomaly — so this restores the dark-table export onto the current (fixed) main AE rather than reviving the stale
aeprod28-rc1.What
Cherry-picked the two dark-table commits cleanly onto
main(no conflicts):ec8382588— 8 dark-vector tables inbuiltinTables(dx_execve,dx_vfs_events,dx_unlink,dx_dlookup,dx_mprotect,dx_creds,dx_bpf,dx_ptrace) + their CH DDL (apply.goOperatorOwnedTables,ddl.go,schema.sql).ddf6d18e3—IsDarkVector+PodEnrichPxLinQueryFor: the dark tables are pid-keyed (noupid), so pod is resolved via aprocess_statspid-merge (bare pod name) rather thanupid_to_pod_name. (Also carries the pgsql firehose timeout from the same commit.)Net export set after this PR is 8 dark + 13 protocol = 21 tables; the 6 control/evidence tables (
adaptive_attribution,ae_reconcile,trigger_watermark,dx_evidence_graph{,_malignant},dx_evidence_manifest) are unchanged.Validation
go build ./src/vizier/services/adaptive_export/...— OKgo test ./src/vizier/services/adaptive_export/...— 14 packages green, 0 failschema.sqlembed already covered byembedsrcson main (dark DDL appends to the existing schema) — no BUILD.bazel change needed.Live validation of the exported dark tables (against deployed tracepoints on a v0.14.20 rig) to follow on the TTL image.