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 16 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
| chDSN := cfg.ClickHouse().NativeDSN() | ||
| // Boot-race: the vizier's plugin service can 404 GetClickHousePlugin for | ||
| // the first few seconds after the AE comes up. Retry a bounded number of | ||
| // times so a cold start doesn't permanently skip plugin enablement. | ||
| var exportURL string | ||
| var err error | ||
| for attempt := 1; attempt <= 5; attempt++ { | ||
| exportURL, err = pluginClient.EnsureClickHousePluginEnabled(chDSN) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the credential-bearing fallback DSN.
When the plugin is newly enabled, EnsureClickHousePluginEnabled returns the supplied fallback unchanged. Since this code now supplies NativeDSN(), the success log at Line 282 emits the ClickHouse username and password through export_url. Log only enabled/status information or a redacted DSN.
Proposed fix
- log.WithField("export_url", exportURL).Info("clickhouse retention plugin is enabled")
+ log.Info("clickhouse retention plugin is enabled")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chDSN := cfg.ClickHouse().NativeDSN() | |
| // Boot-race: the vizier's plugin service can 404 GetClickHousePlugin for | |
| // the first few seconds after the AE comes up. Retry a bounded number of | |
| // times so a cold start doesn't permanently skip plugin enablement. | |
| var exportURL string | |
| var err error | |
| for attempt := 1; attempt <= 5; attempt++ { | |
| exportURL, err = pluginClient.EnsureClickHousePluginEnabled(chDSN) | |
| log.Info("clickhouse retention plugin is enabled") |
🤖 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 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.
| 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.
|
TTL adaptive_export image built ✅ (AE deploys dark-vector bpftraces at boot; schema/event_time + native-DSN fixes) — run 29940139432, from head Pull either tag (same digest AE-only, |
… stream
pxapi's result collector cannot decode the mutation-info response the vizier
returns for a pxtrace deploy ("stream: unimplemented type"), so the deploy Query
always errored even though the UpsertTracepoint applied server-side (verified:
dc_snoop + creds_change reach RUNNING_STATE, dc_snoop exports 220k rows). The
old loop treated that as failure → 5 redundant re-deploys + a false "could not
deploy" warning per tracepoint, and delayed the next tracepoint.
Now the deploy fires once, then confirms success by polling the tracepoint's
OUTPUT TABLE (a plain DataFrame query: "Table not found" = not deployed; compiles
= RUNNING, 0 rows ok). Re-fires the deploy every few attempts as a fallback.
Real success/failure signal, no false negatives, no wasted re-deploys.
|
TTL adaptive_export image built ✅ (confirm tracepoint deploy by table, not the mutation stream) — run 29943812599, from head Pull either tag (same digest AE-only, |
… CH)
End-to-end calibration for the creds_change dark-vector tracepoint (V7). Every
run proves the two properties the trace exists for:
a) the trace WORKS — the AE-deployed commit_creds bpftrace captures a real
privilege escalation (REAL uid >0 -> 0), and
b) attribution reaches ClickHouse — the event flows Pixie -> AE retention
export -> forensic_db.creds_change carrying pid + comm.
Fires deterministically with a stock python:3-slim Job, no custom image:
setresuid(12345,0,0) drops the real uid to a sentinel while KEEPING euid=0
(privileged), then setuid(0) pulls the real uid back to 0 — exactly the
commit_creds(new_uid==0 && old_uid>0) the tracepoint filters for. The sentinel
old_uid=12345 makes the row unambiguous. Asserts the row lands with pid+comm;
pod (pid->pod enrichment) is assert-or-log so it greens automatically later.
Live+e2e gated (AELOAD_LIVE=1 AELOAD_E2E=1). VALIDATED live on rig 6a5fbc75
(aeprod38): count=1 pid=905034 comm=python3 old_uid=12345 new_uid=0, 25s e2e.
The NativeDSN() addition broke gofmt's alignment of the consecutive one-line accessor funcs; golangci's format check (gci/gofmt) flagged config.go. Pure gofmt -w, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.