fix: record edge_prefilter_decisions_total at the policiesActiveFor fork - #295
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a localized observability bug fix that records one existing metric at each prefilter decision without changing forwarding or policy-processing behavior. The added tests cover buffered and streaming paths, route labels, both decisions, and double-count prevention. You can add or adjust custom eligibility rules. Learn more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Detail bug report: View on Detail
Fixes ENG-696
Bug
edge_prefilter_decisions_total{route_kind,decision}was permanently0onHEAD. PR #187 deletedsrc/proxy/server.zig, which held the only production caller ofRuntimeMetrics.recordPrefilterDecision, and the replacementpoliciesActiveFor-based frontends never re-wired the metric. The counter still scraped as well-formed (initializeStaticSeriesseeds all 24 zero series at boot), so it silently degraded from correct (pre-v1.20.0) to identically zero with no error signal, removing operator visibility into how often the edge bypasses the policy engine.Fix
Re-wire
recordPrefilterDecisionat everypoliciesActiveForfork so each piped request records exactly one decision:src/frontend/exec.zig(processBuffered, shared by both frontends' buffered pipes) — recordsfast_path/policy_path; addsrouteKindLabel(signal, format)andbufferedRouteKindLabel(kind)mappers that deriveRouteKindLabelfrom the existingrouteLabel/bufferedRouteLabel(mirroring the pre-feat: use latest policy libs and upgrade to 0.16.0 #187toRouteKindLabel).src/frontend/httpz/server.zigandsrc/frontend/stdio/conn.zig(execPipeStreamin each) — record on both branches of the fork.Decision maps 1:1 to
policiesActiveFor's boolean (false→fast_path,true→policy_path). By-design-zero dimensions (.none,health,passthrough,prometheus_metrics) stay zero, matching the pre-regression behavior for piped traffic.Testing
processBufferedwiring guards that drive a real request and assert the counter increments);zig build test= 524 pass / 1 skip / 0 fail. Verified the two wiring tests fail on an unwired tree (proving they're genuine regression guards, not tautologies).zig buildandzig build -Doptimize=ReleaseSafeexit 0;zig fmt --checkandziglintclean.edgeagainst an echo upstream with a no-policy config then a keep-all log+metric file. Confirmeddatadog_logs/otlp_logs/datadog_metricsincrement onfast_pathwith no policy and onpolicy_pathwith one loaded; siblingedge_policy_records_evaluated_totalincrements on the policy path; no double-count;.none/health/passthrough/prometheus_metricsstay0./api/v2/logs,/v1/logs) on a stdio-frontend build; increments match httpz (parity).edge_requests_total/_responses_total/_duration_seconds/_upstream_attempts_totalall increment as before./api/v2/series(buffered) still returns 202 raw-forwarded, anddatadog_metrics,policy_pathis recorded before the transform fails (the recorded decision is not retracted).datadog-log-bench --listbuilds;upstream-pool-harnesspasses 10/10 against the fixed edge (retry/eviction unchanged).Could not run
/api/v2/series): the stdio-frontend buffered path crashes inexecPipeBuffered→openUpstream(conn.zig:182) on anunreachable. Reproduced identically on an unmodified baseline build, so it is a pre-existing stdio bug outside this fix's scope (the stdio change here is only inexecPipeStream). The buffered wiring is sharedexec.processBuffered, covered by the httpz e2e and the unit wiring test.task test:s3-e2e(MinIO): the environment's Docker registry allows only the officiallibrary/*namespace (alpine/hello-worldpull;minio/miniois denied with "pull access denied"), so the MinIO container the task starts cannot be pulled. The s3-dump code path is untouched by this fix.Automatic Fixes PRs can be configured here.