feat(experimental): pluggable TelemetrySink interface for engine events#1616
feat(experimental): pluggable TelemetrySink interface for engine events#1616jptosso wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR introduces an experimental telemetry API for Coraza WAF observability, including event kind enums, event structs carrying transaction and rule data, telemetry sink interfaces, and internal wiring to emit events throughout WAF initialization and transaction lifecycle phases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1616 +/- ##
==========================================
+ Coverage 87.39% 87.66% +0.27%
==========================================
Files 178 181 +3
Lines 9147 9317 +170
==========================================
+ Hits 7994 8168 +174
+ Misses 879 876 -3
+ Partials 274 273 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jptosso
force-pushed
the
feat/telemetry-sink
branch
from
a6b746a to
dd1005a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
internal/corazawaf/transaction.go (1)
910-918: Minor: shared helper makes truncation events ambiguous for sinks.
setAndReturnBodyLimitInterruptionis invoked for both request-body (HTTP 413) and response-body (HTTP 500) truncation. The emitted message is a single static string, so sinks can distinguish only viatx.lastPhase. Consider passing the direction/status into the message (or a dedicated event field) so operators get actionable context without having to infer phase:- tx.emitEngineError(plugintypes.EngineEventBodyTruncated, nil, "body exceeded configured limit") + tx.emitEngineError(plugintypes.EngineEventBodyTruncated, nil, + fmt.Sprintf("body exceeded configured limit (status %d)", status))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/transaction.go` around lines 910 - 918, The helper setAndReturnBodyLimitInterruption currently emits a static engine event that is ambiguous for request vs response truncation; update it to include direction/status context in the emitted event by adding the HTTP direction or status into the emitEngineError call (e.g., include status and a computed direction string based on the status or tx.lastPhase), or add a dedicated field to the emitted payload so sinks can distinguish request-body (413) vs response-body (500) truncation; change references in setAndReturnBodyLimitInterruption, tx.emitEngineError, and the created types.Interruption to carry that extra context.telemetry/doc.go (1)
4-32: Consider documenting the sink execution contract (synchronous, must not panic/block).The doc covers where sinks live but not how they're called. Since every sink method runs on the request-processing goroutine, an implementation that blocks (e.g., a synchronous HTTP POST) or panics will stall or corrupt a transaction. Worth adding a short "Contract" section:
- Sinks are called synchronously on the request goroutine.
- Sinks must not panic. Recover internally if necessary.
- Long-running work (network I/O, disk writes) should be dispatched to a bounded worker pool owned by the sink; do not block inside the callback.
- Matched data may contain PII; redact/truncate before export.
This aligns with the guideline "Document thread-safety guarantees" and protects users from the transaction-pool-poisoning failure mode flagged on
transaction.go:1672.As per coding guidelines: "Document thread-safety guarantees".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@telemetry/doc.go` around lines 4 - 32, Add a short "Contract" section to the telemetry package docs stating that TelemetrySink implementations (e.g., Noop and any types implementing plugintypes.TelemetrySink) are invoked synchronously on the request-processing goroutine, must not panic (recover internally), must not perform blocking or long-running work directly (dispatch such work to a bounded worker pool or background goroutine), and should redact/truncate any potentially sensitive/matched data before exporting; include a one-line note that violating this contract can stall or corrupt transactions (see transaction handling paths that call sinks).internal/corazawaf/rulegroup.go (1)
163-166: LGTM — clean hot-path gating.A few observations confirming correctness:
- The gate
perRuleTimingEnabled.Load() && ruleTimingSink != nilshort-circuits: a nil timing sink never pays the atomic load cost due to ordering — consider swapping the order for a tiny micro-optimization in the nil-sink case (ruleTimingSink != nil && perRuleTimingEnabled.Load()), since the pointer compare is cheaper than an atomic load on some architectures. Minor / optional.len(tx.matchedRules) > matchCountBeforecorrectly reflects chain-rule semantics sinceMatchRuleappends only on full-chain match.emitPhaseEndunconditionally called with an internal nil-check is fine; the function-call overhead is already accounted for in the +29.8 ns/op benchmark.Also applies to: 265-275, 292-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/rulegroup.go` around lines 163 - 166, The per-rule timing gate currently does an atomic load before checking the sink pointer (perRuleTimingEnabled.Load() && tx.WAF.ruleTimingSink != nil); swap the operands to check the pointer first (tx.WAF.ruleTimingSink != nil && tx.WAF.perRuleTimingEnabled.Load()) to avoid the atomic load when the sink is nil; apply the same reordering to the other occurrences flagged (the checks around lines 265-275 and 292-294), leaving semantics unchanged for functions like MatchRule, the perRuleTiming variable, and emitPhaseEnd.experimental/telemetry_sink_test.go (1)
285-312: Nit:TestTelemetrySink_NilShimReturnsInputConfigname is misleading.The test doesn't pass a nil config — it passes a non-
wafConfigimplementation and asserts the shim returns it unchanged. A name likeTestTelemetrySink_UnsupportedConfig_ReturnsInputUnchangedwould better describe what's being verified (and matches the companion test at line 300PerRuleTimingShims_Fallback). Purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/telemetry_sink_test.go` around lines 285 - 312, Rename the misleading test TestTelemetrySink_NilShimReturnsInputConfig to TestTelemetrySink_UnsupportedConfig_ReturnsInputUnchanged and update any references, so the name accurately reflects that it passes a non-wafConfig implementation and expects the shim (experimental.WAFConfigWithTelemetrySink) to return the input unchanged; keep the test body unchanged and mirror the naming style used by the companion test TestTelemetrySink_PerRuleTimingShims_Fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experimental/telemetry_sink_test.go`:
- Line 501: The call to tx.ProcessRequestBody() ignores its returned error
causing golangci-lint errcheck failure; update the test to capture and handle
the error from tx.ProcessRequestBody() (e.g., err := tx.ProcessRequestBody())
and assert or fail on non-nil error (use t.Fatalf/t.Fatalf-like helper or
require.NoError/AssertNoError depending on test helpers in use) so the error is
explicitly checked and reported.
In `@internal/corazawaf/telemetry.go`:
- Around line 20-27: SetTelemetrySink currently writes w.TelemetrySink and
w.ruleTimingSink without synchronization while
emitTransactionStart/emitRuleMatch/emitRuleTimed read them concurrently,
creating a data race; fix by documenting the thread-safety guarantee: add a
clear doc comment on the WAF.SetTelemetrySink method stating it MUST be called
only at startup (before the WAF is published to any transaction) and is not safe
to call concurrently with active transactions—ensure this matches how NewWAF
wires the sink and mention that perRuleTimingEnabled remains the
runtime-toggleable knob; alternatively, if runtime reconfiguration is desired
later, replace those fields with
atomic.Pointer[plugintypes.TelemetrySink]/atomic.Pointer[plugintypes.RuleTimingSink]
and have the emit helpers call Load() once.
- Around line 141-142: The file ends with extra blank line(s) after the final
closing brace '}', which causes gofmt/golangci-lint failures; remove the
trailing blank lines so the file terminates with a single newline immediately
following the last '}' and then run gofmt -w on the file (or go fmt ./...) and
commit the formatted file to resolve the CI lint error.
In `@internal/corazawaf/transaction.go`:
- Around line 1671-1674: The Close method currently calls
emitTransactionFinish() before cleanup which allows a panic in
sink.OnTransaction to skip tx.variables.reset(), tx.requestBodyBuffer.Reset(),
tx.responseBodyBuffer.Reset() and poison tx.WAF.txPool.Put(tx); fix by either
moving the call to emitTransactionFinish() to after all cleanup/reset calls in
Transaction.Close (so cleanup always runs before txPool.Put) or wrap
emitTransactionFinish() with a panic-safe defer recover() inside
emitTransactionFinish itself (telemetry sink isolation) so a sink panic is
logged but does not abort cleanup; also apply the same panic-guard pattern to
other emit* call sites (rule match, engine error, phase end) if you choose the
recover approach.
In `@internal/corazawaf/waf.go`:
- Around line 136-149: Update the WAF struct doc to explicitly exempt
telemetry-related fields (TelemetrySink, ruleTimingSink, perRuleTimingEnabled)
from the “immutable fields” rule: state that these three fields are
intentionally runtime-mutable and explain that perRuleTimingEnabled is toggled
atomically via SetPerRuleTimingEnabled and that TelemetrySink must not be
assigned directly (use SetTelemetrySink so ruleTimingSink stays synchronized).
Also add a short note warning readers that direct writes to TelemetrySink or
ruleTimingSink can create race conditions and that SetTelemetrySink handles the
required consistency between those two fields.
In `@telemetry/noop_test.go`:
- Around line 13-20: Replace the redundant typed variable declaration in
TestNoopSatisfiesInterface with a regular short declaration for the runtime
calls and add a compile-time blank identifier assertion to show the interface
satisfaction: change the line "var s plugintypes.TelemetrySink =
telemetry.Noop()" to "s := telemetry.Noop()" and add "var _
plugintypes.TelemetrySink = telemetry.Noop()" (or the equivalent blank
assertion) so the test still calls s.OnEngineReady / s.OnTransaction /
s.OnRuleMatch / s.OnEngineError while avoiding staticcheck ST1023 and making the
interface satisfaction explicit.
In `@types/phase.go`:
- Around line 30-46: The test output changed because RulePhase now implements
fmt.Stringer via RulePhase.String(), so update the failing test assertion to
print or compare the numeric phase explicitly: in the test referencing m.Phase
(e.g., experimental/telemetry_sink_test.go test that does t.Fatalf("phase = %v",
m.Phase)), change the format to use %d with int(m.Phase) or otherwise convert
m.Phase to an int before formatting/comparing; this keeps the test output stable
while preserving RulePhase.String().
---
Nitpick comments:
In `@experimental/telemetry_sink_test.go`:
- Around line 285-312: Rename the misleading test
TestTelemetrySink_NilShimReturnsInputConfig to
TestTelemetrySink_UnsupportedConfig_ReturnsInputUnchanged and update any
references, so the name accurately reflects that it passes a non-wafConfig
implementation and expects the shim (experimental.WAFConfigWithTelemetrySink) to
return the input unchanged; keep the test body unchanged and mirror the naming
style used by the companion test TestTelemetrySink_PerRuleTimingShims_Fallback.
In `@internal/corazawaf/rulegroup.go`:
- Around line 163-166: The per-rule timing gate currently does an atomic load
before checking the sink pointer (perRuleTimingEnabled.Load() &&
tx.WAF.ruleTimingSink != nil); swap the operands to check the pointer first
(tx.WAF.ruleTimingSink != nil && tx.WAF.perRuleTimingEnabled.Load()) to avoid
the atomic load when the sink is nil; apply the same reordering to the other
occurrences flagged (the checks around lines 265-275 and 292-294), leaving
semantics unchanged for functions like MatchRule, the perRuleTiming variable,
and emitPhaseEnd.
In `@internal/corazawaf/transaction.go`:
- Around line 910-918: The helper setAndReturnBodyLimitInterruption currently
emits a static engine event that is ambiguous for request vs response
truncation; update it to include direction/status context in the emitted event
by adding the HTTP direction or status into the emitEngineError call (e.g.,
include status and a computed direction string based on the status or
tx.lastPhase), or add a dedicated field to the emitted payload so sinks can
distinguish request-body (413) vs response-body (500) truncation; change
references in setAndReturnBodyLimitInterruption, tx.emitEngineError, and the
created types.Interruption to carry that extra context.
In `@telemetry/doc.go`:
- Around line 4-32: Add a short "Contract" section to the telemetry package docs
stating that TelemetrySink implementations (e.g., Noop and any types
implementing plugintypes.TelemetrySink) are invoked synchronously on the
request-processing goroutine, must not panic (recover internally), must not
perform blocking or long-running work directly (dispatch such work to a bounded
worker pool or background goroutine), and should redact/truncate any potentially
sensitive/matched data before exporting; include a one-line note that violating
this contract can stall or corrupt transactions (see transaction handling paths
that call sinks).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a82c7ae9-6b53-4d3e-98e4-1e1d5ffd4c8f
📒 Files selected for processing (14)
config.goexperimental/plugins/plugintypes/telemetry.goexperimental/telemetry_sink.goexperimental/telemetry_sink_test.gointernal/corazawaf/rulegroup.gointernal/corazawaf/telemetry.gointernal/corazawaf/transaction.gointernal/corazawaf/waf.gotelemetry/bench_test.gotelemetry/doc.gotelemetry/noop.gotelemetry/noop_test.gotypes/phase.gowaf.go
| func TestNoopSatisfiesInterface(t *testing.T) { | ||
| var s plugintypes.TelemetrySink = telemetry.Noop() | ||
| // Methods must be safe to call with zero-value events. | ||
| s.OnEngineReady(plugintypes.EngineReadyEvent{}) | ||
| s.OnTransaction(plugintypes.TransactionEvent{}) | ||
| s.OnRuleMatch(plugintypes.RuleMatchEvent{}) | ||
| s.OnEngineError(plugintypes.EngineEvent{}) | ||
| } |
There was a problem hiding this comment.
Fix staticcheck ST1023 and strengthen the interface assertion.
telemetry.Noop() already returns plugintypes.TelemetrySink, so the explicit type on the LHS is redundant (staticcheck ST1023, CI failure). A compile-time blank assertion expresses the "satisfies interface" intent more clearly than a runtime variable with an explicit type.
🔧 Proposed fix
func TestNoopSatisfiesInterface(t *testing.T) {
- var s plugintypes.TelemetrySink = telemetry.Noop()
+ // Compile-time check that Noop() satisfies the sink contract.
+ var _ plugintypes.TelemetrySink = telemetry.Noop()
+ s := telemetry.Noop()
// Methods must be safe to call with zero-value events.
s.OnEngineReady(plugintypes.EngineReadyEvent{})
s.OnTransaction(plugintypes.TransactionEvent{})
s.OnRuleMatch(plugintypes.RuleMatchEvent{})
s.OnEngineError(plugintypes.EngineEvent{})
}📝 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.
| func TestNoopSatisfiesInterface(t *testing.T) { | |
| var s plugintypes.TelemetrySink = telemetry.Noop() | |
| // Methods must be safe to call with zero-value events. | |
| s.OnEngineReady(plugintypes.EngineReadyEvent{}) | |
| s.OnTransaction(plugintypes.TransactionEvent{}) | |
| s.OnRuleMatch(plugintypes.RuleMatchEvent{}) | |
| s.OnEngineError(plugintypes.EngineEvent{}) | |
| } | |
| func TestNoopSatisfiesInterface(t *testing.T) { | |
| // Compile-time check that Noop() satisfies the sink contract. | |
| var _ plugintypes.TelemetrySink = telemetry.Noop() | |
| s := telemetry.Noop() | |
| // Methods must be safe to call with zero-value events. | |
| s.OnEngineReady(plugintypes.EngineReadyEvent{}) | |
| s.OnTransaction(plugintypes.TransactionEvent{}) | |
| s.OnRuleMatch(plugintypes.RuleMatchEvent{}) | |
| s.OnEngineError(plugintypes.EngineEvent{}) | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 14-14:
ST1023: should omit type plugintypes.TelemetrySink from declaration; it will be inferred from the right-hand side (staticcheck)
🪛 golangci-lint (2.11.4)
[error] 14-14: ST1023: should omit type plugintypes.TelemetrySink from declaration; it will be inferred from the right-hand side
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@telemetry/noop_test.go` around lines 13 - 20, Replace the redundant typed
variable declaration in TestNoopSatisfiesInterface with a regular short
declaration for the runtime calls and add a compile-time blank identifier
assertion to show the interface satisfaction: change the line "var s
plugintypes.TelemetrySink = telemetry.Noop()" to "s := telemetry.Noop()" and add
"var _ plugintypes.TelemetrySink = telemetry.Noop()" (or the equivalent blank
assertion) so the test still calls s.OnEngineReady / s.OnTransaction /
s.OnRuleMatch / s.OnEngineError while avoiding staticcheck ST1023 and making the
interface satisfaction explicit.
c4a5398 to
fe163f5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
internal/corazawaf/waf.go (1)
136-149:⚠️ Potential issue | 🟡 MinorWAF struct doc still claims all fields are immutable, but these three are designed for runtime mutation.
Lines 43-51 still state "All WAF instance fields are immutable, if you update any of them in runtime you might create concurrency issues." That contradicts
TelemetrySink/ruleTimingSink(mutated together viaSetTelemetrySink) andperRuleTimingEnabled(toggled atomically viaSetPerRuleTimingEnabled). Please carve out these three fields in the struct-level comment and add a warning onTelemetrySinkthat callers must not assign it directly (useSetTelemetrySinkto keepruleTimingSinkin sync).As per coding guidelines: "Document thread-safety guarantees".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/waf.go` around lines 136 - 149, Update the WAF struct comment to document thread-safety exceptions: explicitly call out that TelemetrySink, ruleTimingSink and perRuleTimingEnabled are mutable at runtime and must not be modified directly; instruct callers to use SetTelemetrySink (so TelemetrySink and ruleTimingSink remain in sync) and SetPerRuleTimingEnabled (atomic toggle) instead, and add a short warning next to the TelemetrySink field that direct assignment is forbidden.telemetry/noop_test.go (1)
15-22:⚠️ Potential issue | 🟡 MinorCI lint (ST1023) still failing — redundant type on LHS.
telemetry.Noop()already returnsplugintypes.TelemetrySink, so the explicit type here still trips staticcheck ST1023 and theGitHub Check: lintjob. Switch to a blank-identifier compile-time assertion (which expresses the "satisfies interface" intent more strongly) plus an inferred short declaration for the runtime calls.🔧 Proposed fix
func TestNoopSatisfiesInterface(t *testing.T) { - var s plugintypes.TelemetrySink = telemetry.Noop() + // Compile-time check that Noop() satisfies the sink contract. + var _ plugintypes.TelemetrySink = telemetry.Noop() + s := telemetry.Noop() // Methods must be safe to call with zero-value events. s.OnEngineReady(plugintypes.EngineReadyEvent{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@telemetry/noop_test.go` around lines 15 - 22, Replace the redundant explicit type assertion in TestNoopSatisfiesInterface: remove "var s plugintypes.TelemetrySink = telemetry.Noop()" and instead add a compile-time interface satisfaction like "var _ plugintypes.TelemetrySink = telemetry.Noop()" and use a short variable declaration for runtime calls (e.g., "s := telemetry.Noop()") so calls to s.OnEngineReady, s.OnTransaction, s.OnRuleMatch, and s.OnEngineError remain valid while satisfying staticcheck ST1023.internal/corazawaf/telemetry.go (2)
20-27:⚠️ Potential issue | 🟠 MajorThread-safety of
SetTelemetrySinkstill undocumented — and the two field writes are not atomic.
SetTelemetrySinkwritesw.TelemetrySinkandw.ruleTimingSinkas plain interface fields, whileemitTransactionStart/emitRuleMatch/emitRuleTimed/emitEngineError/emitPhaseEnd/emitTransactionFinishread them concurrently from transaction goroutines. Interface values are two-word fat pointers, so a call made on a live WAF can produce a torn read and crash — this is a data race per the Go memory model.Two acceptable fixes (pick one):
- Document that
SetTelemetrySinkis startup-only (must be called before the WAF is handed out to any caller producing transactions), matching howNewWAFwires the sink today;perRuleTimingEnabledremains the runtime-toggleable knob.- Or back both fields with
atomic.Pointer[plugintypes.TelemetrySink]/atomic.Pointer[plugintypes.RuleTimingSink]and have the emit helpersLoad()once.As per coding guidelines: "Document thread-safety guarantees".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/telemetry.go` around lines 20 - 27, SetTelemetrySink currently writes WAF.TelemetrySink and w.ruleTimingSink non-atomically while emitTransactionStart / emitRuleMatch / emitRuleTimed / emitEngineError / emitPhaseEnd / emitTransactionFinish read them concurrently; either document that SetTelemetrySink is startup-only (callable only before handing WAF to any goroutine, matching NewWAF wiring and keeping perRuleTimingEnabled as the runtime toggle) or make the fields safe for concurrent access by replacing TelemetrySink and ruleTimingSink with atomic.Pointer[plugintypes.TelemetrySink] / atomic.Pointer[plugintypes.RuleTimingSink] and update SetTelemetrySink to Store() the pointers and all emit helpers to Load() once per emission; add a one-line doc comment on SetTelemetrySink stating the chosen thread-safety guarantee.
138-140:⚠️ Potential issue | 🔴 CriticalCI still failing on gofmt — trailing blank line after the final
}.The lint pipeline continues to flag this file at line 139 (
File is not properly formatted (gofmt)). Rungofmt -w internal/corazawaf/telemetry.go(orgo fmt ./...) and commit. As per coding guidelines: "Usegofmtandgolintfor code formatting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/telemetry.go` around lines 138 - 140, The file ends with an extra trailing blank line after the final closing brace '}', causing gofmt to fail; remove the blank line and reformat the code (run gofmt -w or go fmt ./...) so the file is properly formatted, then commit the change.internal/corazawaf/transaction.go (1)
1671-1674:⚠️ Potential issue | 🟠 MajorSink panic in
emitTransactionFinishcan still poison the txPool.
emitTransactionFinish()still runs synchronously before any cleanup inClose(). A panic from a user-supplied sink will unwind through thedefer tx.WAF.txPool.Put(tx)but skiptx.variables.reset(),tx.requestBodyBuffer.Reset(), andtx.responseBodyBuffer.Reset()(Lines 1695-1701), handing a dirtyTransactionback to the pool (stale buffers, undeleted temp files, inconsistent collections).Either move the emit to after cleanup, or wrap the helper with
defer recover()centrally intelemetry.go.🛡️ Option A — emit after cleanup
func (tx *Transaction) Close() error { - tx.emitTransactionFinish() defer tx.WAF.txPool.Put(tx) + defer tx.emitTransactionFinish() var errs []error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazawaf/transaction.go` around lines 1671 - 1674, The call to emitTransactionFinish() in Transaction.Close() can panic and bypass cleanup, returning a poisoned Transaction to txPool; to fix, ensure emitTransactionFinish cannot skip cleanup by either moving the emitTransactionFinish() invocation to after all cleanup steps (variables.reset(), requestBodyBuffer.Reset(), responseBodyBuffer.Reset(), temp file removals) in Transaction.Close(), or wrap the emitTransactionFinish call with a recover-protected helper in telemetry.go (e.g., a safeEmitTransactionFinish wrapper that defers a recover and logs the panic) and call that wrapper instead; reference the Transaction.Close method and emitTransactionFinish function when making the change.
🧹 Nitpick comments (1)
experimental/telemetry_sink.go (1)
1-87: LGTM — clean capability-interface pattern, well-scoped experimental API.Private capability interfaces (
wafConfigWithTelemetrySink,wafConfigWithPerRuleTiming,wafWithPerRuleTimingToggle) cleanly keep the telemetry contract off the stablecoraza.WAFConfig/coraza.WAFsurface while still letting external callers wire it up. The doc comments explicitly mark each helper as experimental and may-change-before-v4, andWAFEnablePerRuleTimingcorrectly returnsboolso callers can detect third-party WAFs that don't implement the toggle.One minor consistency nit:
WAFConfigWithTelemetrySink/WAFConfigWithPerRuleTimingsilently return the input on a missing capability whileWAFEnablePerRuleTimingreturnsfalse. The silent path is documented, but if an embedder passes a mock/third-partyWAFConfigthey'd get zero feedback that telemetry was dropped. Consider adding a similar boolean signal on the two config helpers in a follow-up, or document the trade-off more explicitly. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/telemetry_sink.go` around lines 1 - 87, Add non-breaking boolean-feedback helpers to surface when a capability was honoured: implement WAFConfigWithTelemetrySinkOK(cfg coraza.WAFConfig, sink plugintypes.TelemetrySink) (coraza.WAFConfig, bool) and WAFConfigWithPerRuleTimingOK(cfg coraza.WAFConfig, enabled bool) (coraza.WAFConfig, bool) that mirror the current WAFConfigWithTelemetrySink and WAFConfigWithPerRuleTiming logic but return true when the cfg supports the corresponding private interfaces (wafConfigWithTelemetrySink, wafConfigWithPerRuleTiming) and false otherwise; update the doc comments for these new functions to mention they provide explicit feedback without changing the existing helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/corazawaf/telemetry.go`:
- Around line 20-27: SetTelemetrySink currently writes WAF.TelemetrySink and
w.ruleTimingSink non-atomically while emitTransactionStart / emitRuleMatch /
emitRuleTimed / emitEngineError / emitPhaseEnd / emitTransactionFinish read them
concurrently; either document that SetTelemetrySink is startup-only (callable
only before handing WAF to any goroutine, matching NewWAF wiring and keeping
perRuleTimingEnabled as the runtime toggle) or make the fields safe for
concurrent access by replacing TelemetrySink and ruleTimingSink with
atomic.Pointer[plugintypes.TelemetrySink] /
atomic.Pointer[plugintypes.RuleTimingSink] and update SetTelemetrySink to
Store() the pointers and all emit helpers to Load() once per emission; add a
one-line doc comment on SetTelemetrySink stating the chosen thread-safety
guarantee.
- Around line 138-140: The file ends with an extra trailing blank line after the
final closing brace '}', causing gofmt to fail; remove the blank line and
reformat the code (run gofmt -w or go fmt ./...) so the file is properly
formatted, then commit the change.
In `@internal/corazawaf/transaction.go`:
- Around line 1671-1674: The call to emitTransactionFinish() in
Transaction.Close() can panic and bypass cleanup, returning a poisoned
Transaction to txPool; to fix, ensure emitTransactionFinish cannot skip cleanup
by either moving the emitTransactionFinish() invocation to after all cleanup
steps (variables.reset(), requestBodyBuffer.Reset(), responseBodyBuffer.Reset(),
temp file removals) in Transaction.Close(), or wrap the emitTransactionFinish
call with a recover-protected helper in telemetry.go (e.g., a
safeEmitTransactionFinish wrapper that defers a recover and logs the panic) and
call that wrapper instead; reference the Transaction.Close method and
emitTransactionFinish function when making the change.
In `@internal/corazawaf/waf.go`:
- Around line 136-149: Update the WAF struct comment to document thread-safety
exceptions: explicitly call out that TelemetrySink, ruleTimingSink and
perRuleTimingEnabled are mutable at runtime and must not be modified directly;
instruct callers to use SetTelemetrySink (so TelemetrySink and ruleTimingSink
remain in sync) and SetPerRuleTimingEnabled (atomic toggle) instead, and add a
short warning next to the TelemetrySink field that direct assignment is
forbidden.
In `@telemetry/noop_test.go`:
- Around line 15-22: Replace the redundant explicit type assertion in
TestNoopSatisfiesInterface: remove "var s plugintypes.TelemetrySink =
telemetry.Noop()" and instead add a compile-time interface satisfaction like
"var _ plugintypes.TelemetrySink = telemetry.Noop()" and use a short variable
declaration for runtime calls (e.g., "s := telemetry.Noop()") so calls to
s.OnEngineReady, s.OnTransaction, s.OnRuleMatch, and s.OnEngineError remain
valid while satisfying staticcheck ST1023.
---
Nitpick comments:
In `@experimental/telemetry_sink.go`:
- Around line 1-87: Add non-breaking boolean-feedback helpers to surface when a
capability was honoured: implement WAFConfigWithTelemetrySinkOK(cfg
coraza.WAFConfig, sink plugintypes.TelemetrySink) (coraza.WAFConfig, bool) and
WAFConfigWithPerRuleTimingOK(cfg coraza.WAFConfig, enabled bool)
(coraza.WAFConfig, bool) that mirror the current WAFConfigWithTelemetrySink and
WAFConfigWithPerRuleTiming logic but return true when the cfg supports the
corresponding private interfaces (wafConfigWithTelemetrySink,
wafConfigWithPerRuleTiming) and false otherwise; update the doc comments for
these new functions to mention they provide explicit feedback without changing
the existing helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5b2cab4-6e54-4e03-8e36-298bd281623e
📒 Files selected for processing (14)
config.goexperimental/plugins/plugintypes/telemetry.goexperimental/telemetry_sink.goexperimental/telemetry_sink_test.gointernal/corazawaf/rulegroup.gointernal/corazawaf/telemetry.gointernal/corazawaf/transaction.gointernal/corazawaf/waf.gotelemetry/bench_test.gotelemetry/doc.gotelemetry/noop.gotelemetry/noop_test.gotypes/phase.gowaf.go
✅ Files skipped from review due to trivial changes (1)
- telemetry/doc.go
🚧 Files skipped from review as they are similar to previous changes (5)
- types/phase.go
- telemetry/noop.go
- waf.go
- experimental/plugins/plugintypes/telemetry.go
- experimental/telemetry_sink_test.go
jptosso
force-pushed
the
feat/telemetry-sink
branch
from
fe163f5 to
0cda0a4
Compare
Adds an opt-in TelemetrySink interface so connectors (coraza-caddy,
coraza-proxy-wasm, coraza-spoa, libcoraza) can route engine events into
their native observability stacks. Core ships the hooks only; no
Prometheus, OpenTelemetry, or log-schema dependency is added to go.mod.
Public surface (under experimental/):
- plugintypes.TelemetrySink (4-method interface)
- plugintypes.RuleTimingSink (optional extension for per-rule timing)
- TransactionEvent, RuleMatchEvent, EngineEvent, EngineReadyEvent,
RuleTimingEvent
- experimental.WAFConfigWithTelemetrySink(cfg, sink)
- experimental.WAFConfigWithPerRuleTiming(cfg, bool)
- experimental.WAFEnablePerRuleTiming(waf, bool) runtime toggle
- telemetry.Noop() explicit no-op sink
- types.RulePhase.String() stable label value
Engine wiring is at six existing seams (transaction start, phase end,
rule match, transaction finish, body-processor error, body-limit
truncation). Per-rule timing lives behind an atomic.Bool gate so the
off-state cost is one atomic load per phase.
Performance for non-opt-in users (measured against upstream main,
benchtime=5s, 10 runs, Apple M4):
main : 3241.6 ns/op, 74 allocs
this PR : 3271.4 ns/op, 74 allocs
delta : +29.8 ns/op (+0.92%), 0 extra allocs
The ~30ns delta is six nil-pointer checks plus one atomic load per
phase. No heap allocations, no behavior change.
Full suite: 26/26 packages pass with -race. 100% coverage on every
experimental shim and every engine emit helper. TinyGo wasip1 build
verified.
Claude Code Opus 4.7 was used in the development of this change.
jptosso
force-pushed
the
feat/telemetry-sink
branch
from
0cda0a4 to
fdc4820
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in, dependency-free telemetry hook for Coraza engine events (transactions, rule matches, engine errors, and optional per-rule timings) via an experimental/plugins/plugintypes.TelemetrySink interface, enabling downstream connectors to export metrics/traces/logs in their native observability stacks.
Changes:
- Wire a pluggable
TelemetrySinkinto WAF construction and transaction/rule evaluation paths, emitting structured engine events. - Add opt-in per-rule timing with a runtime-toggleable
atomic.Boolgate and an optionalRuleTimingSinkextension interface. - Add stable string representations for
types.RulePhasefor safer metric labeling, plus comprehensive tests/benchmarks for the telemetry surface.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
waf.go |
Attaches configured telemetry sink / per-rule timing at WAF init; emits EngineReady event. |
config.go |
Adds internal config fields + private WithTelemetrySink/WithPerRuleTiming setters. |
types/phase.go |
Adds RulePhase.String() for stable label/log identifiers. |
internal/corazawaf/waf.go |
Adds telemetry fields + per-rule timing flag; emits transaction start. |
internal/corazawaf/transaction.go |
Emits rule-match events, body processor errors, and body truncation engine events; emits transaction finish on Close(). |
internal/corazawaf/rulegroup.go |
Emits per-phase end transaction events and optional per-rule timing events. |
internal/corazawaf/telemetry.go |
Centralizes sink attachment and telemetry emission helpers. |
experimental/telemetry_sink.go |
Adds experimental shims for wiring sinks and toggling per-rule timing + noop sink. |
experimental/plugins/plugintypes/telemetry.go |
Defines sink interfaces and event structs/enums (public experimental contract). |
experimental/telemetry_sink_test.go |
Integration tests covering emit seams, semantics, and shims. |
experimental/telemetry_sink_bench_test.go |
Benchmarks for baseline/noop/counting/per-rule on-off overhead. |
| ruleStart := time.Now().UnixNano() | ||
| matchCountBefore := len(tx.matchedRules) | ||
| r.Evaluate(phase, tx, transformationCache) | ||
| tx.emitRuleTimed(r, | ||
| time.Duration(time.Now().UnixNano()-ruleStart), |
There was a problem hiding this comment.
Per-rule timing and phase timing are computed using time.Now().UnixNano() deltas. UnixNano strips Go's monotonic clock reading, so NTP/clock adjustments can produce negative or inaccurate durations that will be exported to telemetry sinks. Prefer capturing a time.Time start (time.Now()) and using time.Since(start) / time.Now().Sub(start) for both rule timing and phase duration emission so the monotonic component is preserved.
| ruleStart := time.Now().UnixNano() | |
| matchCountBefore := len(tx.matchedRules) | |
| r.Evaluate(phase, tx, transformationCache) | |
| tx.emitRuleTimed(r, | |
| time.Duration(time.Now().UnixNano()-ruleStart), | |
| ruleStart := time.Now() | |
| matchCountBefore := len(tx.matchedRules) | |
| r.Evaluate(phase, tx, transformationCache) | |
| tx.emitRuleTimed(r, | |
| time.Since(ruleStart), |
| Status: status, | ||
| Action: "deny", | ||
| } | ||
| tx.emitEngineError(plugintypes.EngineEventBodyTruncated, nil, "body exceeded configured limit") |
There was a problem hiding this comment.
The emitted EngineEventBodyTruncated message is always "body exceeded configured limit", but the same helper is used for both request-body (413) and response-body (500) truncation. This makes the telemetry hard to action because operators can’t tell which limit to tune. Consider including whether it was the request or response body in the message (or pass that info into the helper) so sinks can expose clearer alerts/logs.
| tx.emitEngineError(plugintypes.EngineEventBodyTruncated, nil, "body exceeded configured limit") | |
| message := "body exceeded configured limit" | |
| switch status { | |
| case 413: | |
| message = "request body exceeded configured limit" | |
| case 500: | |
| message = "response body exceeded configured limit" | |
| } | |
| tx.emitEngineError(plugintypes.EngineEventBodyTruncated, nil, message) |
| // TestTelemetrySink_ContextPropagation: if the caller creates a transaction | ||
| // with a specific context, that context must surface in every tx event. This | ||
| // is the APM span-chaining contract. | ||
| func TestTelemetrySink_ContextPropagation(t *testing.T) { | ||
| sink := &recordingSink{} | ||
| cfg := coraza.NewWAFConfig().WithDirectives(`SecRuleEngine On`) | ||
| cfg = experimental.WAFConfigWithTelemetrySink(cfg, sink) | ||
| waf, err := coraza.NewWAF(cfg) | ||
| if err != nil { | ||
| t.Fatalf("new waf: %v", err) | ||
| } | ||
| tx := waf.NewTransactionWithID("my-request-id") | ||
| tx.ProcessURI("/", "GET", "HTTP/1.1") | ||
| tx.ProcessRequestHeaders() | ||
| tx.ProcessLogging() | ||
| _ = tx.Close() |
There was a problem hiding this comment.
TestTelemetrySink_ContextPropagation doesn’t actually set a custom context (it uses NewTransactionWithID, which only changes the transaction ID). As written it only asserts TransactionID propagation, so it won’t catch regressions where a non-default context isn’t carried into events. Either set a non-background context via NewTransactionWithOptions (and assert it is the same in events) or rename the test to reflect what it verifies.
|
@coderabbitai full_review |
|
✅ Actions performedFull review triggered. |
Summary
Adds a minimal, opt-in
TelemetrySinkinterface that lets connectors (coraza-caddy, coraza-proxy-wasm, coraza-spoa, libcoraza) route coraza engine events into their native observability stacks. Coraza core ships the hooks; it does not ship a Prometheus client, an OTel SDK, or any log schema. Zero new dependencies ingo.mod.Performance impact for non-opt-in users
Measured on Apple M4,
go test -bench -benchtime=5s -count=10, identical 2-rule transaction shape on both branches:The ~30 ns delta corresponds to 6 nil-pointer checks at phase boundaries plus one
atomic.Bool.Load()per phase. No heap allocation, no semantic change when no sink is attached.Motivation
Today coraza is invisible to operators. No phase timings, no per-rule match counters, no structured hook for "is the engine healthy." The existing
ErrorLogCbis called only on matched rules withlog:true— an operator cannot answer "is coraza 30 ms of my 40 ms tail?" or "did my rule change double the block rate?" without writing bespoke glue.Events emitted
Core ships five events. Sinks turn them into metrics, logs, or spans in the operator's native stack. Coraza makes no assumption about which.
EngineReadyEvent— once per WAF, at end ofNewWAFRulesLoadedintInitDurationtime.DurationNewWAFTransactionEvent— at transaction start, each phase end, and transaction closeKindTransactionEventKindstart|phase_end|finishTransactionIDstringContextcontext.ContextPhasetypes.RulePhasephase_endPhaseDurationtime.DurationRulesEvaluatedintInterruption*types.Interruptionfinish; current state onphase_endRuleMatchEvent— once per rule match, synchronously after match record is appendedTransactionIDstringContextcontext.ContextRuletypes.RuleMetadataPhasetypes.RulePhaseActionstring"deny"|"drop"|"pass"|"allow"|"redirect"|""(no disruptive action)EnforcedboolData[]types.MatchDataEngineEvent— on engine-internal failuresKindEngineEventKindinit|parse|body_processor|body_truncated|transactionTransactionIDstringPhasetypes.RulePhaseErrerrorMessagestringRuleTimingEvent— opt-in per-rule timing (see dedicated section below)TransactionIDstringRuletypes.RuleMetadataPhasetypes.RulePhaseDurationtime.DurationMatchedboolPer-rule timing — opt-in, investigation-only
Per-rule timing answers the question "which specific rule is making phase 2 slow?". It is deliberately separated from the base telemetry for three reasons: high cardinality (one event per rule per request — at CRS full that's ~450), high frequency, and measurable hot-path cost when active.
How to enable it
Two APIs, one atomic flag under the hood.
At startup:
At runtime — flip on a live WAF when an alert fires, flip off when investigation is done:
Safe to call concurrently with active transactions; the change is observed on the next rule-evaluation loop.
How the sink consumes it
RuleTimingEventis delivered via a separate optional interface. A sink opts in by implementing bothTelemetrySinkandRuleTimingSink:A sink that does NOT implement
RuleTimingSinkis fine — the engine type-asserts once atSetTelemetrySinktime and caches the result, so sinks without the extension pay zero runtime cost even when the flag is toggled on.Performance
atomic.Bool.Load()+ cached pointer comparetime.Now()calls + one interface dispatchAt CRS paranoia 2 (~450 rules across phases) the on-state cost is roughly +34 µs per transaction — around 10% of baseline. Acceptable for a 5-minute debug window, too much for always-on. The off-state cost is sub-1% and was measured at +1.0% over the counting sink across 5 benchmark runs — within noise.
When to use it
TransactionEvent.PhaseDuration+RulesEvaluatedinsteadOperator use cases → events
TransactionEvent{Kind=finish, Interruption!=nil}→requests_total{decision="blocked"}counterRuleMatchEvent{Action="deny"|"drop"|"redirect", Enforced=true}→ per-minute rateRuleMatchEventaggregated byRule.ID()→ top-N over timeRuleMatchEvent{Action="deny", Enforced=false}→ would-have-blocked rateTransactionEvent{Kind=phase_end, PhaseDuration, RulesEvaluated}→ p99 histogram per phaseRuleTimingEvent.DurationperRule.ID()EngineEventstream grouped byKindEngineEvent{Kind=body_truncated}counter; spike = raise limitEngineReadyEvent{RulesLoaded, InitDuration}→ info gaugeEngineReadyEvent.InitDurationon each instance bootcontext.ContextthroughNewTransactionWithOptions; OTel sink startscoraza.evaluatespan chained to parent traceEngineReadyEvent.RulesLoadeddelta after deploysExample — wiring into coraza-caddy
To expose these events as Prometheus counters on Caddy's existing
/metricsendpoint, a Caddy module attaches a sink in itsbuildWAF:No new scrape target, no new registry — the series appear on Caddy's built-in
/metricsendpoint ascaddy_waf_requests_total. Operators addrate(caddy_waf_requests_total{decision="blocked"}[5m])to their existing dashboard. Same pattern works for Envoy stats (proxy-wasm), HAProxy SPOP (spoa), zap, slog, OpenTelemetry — each connector maps events to its native telemetry primitives.What's in
Public surface (all under
experimental/or additive on existing types)experimental/plugins/plugintypes.TelemetrySink— 4-method interface:OnEngineReady,OnTransaction,OnRuleMatch,OnEngineErrorexperimental/plugins/plugintypes.RuleTimingSink— optional extension interface for opt-in per-rule timingTransactionEvent,RuleMatchEvent,EngineEvent,EngineReadyEvent,RuleTimingEventexperimental.WAFConfigWithTelemetrySink(cfg, sink)experimental.WAFConfigWithPerRuleTiming(cfg, bool)experimental.WAFEnablePerRuleTiming(waf, bool)— runtime toggleexperimental.NoopTelemetrySink()— explicit no-op for callers that prefer a non-nil value (the engine treatsnilas no-op for free)types.RulePhase.String()— stable label value for metricsInternal changes
internal/corazawaf.WAFgainsTelemetrySink, cachedruleTimingSink(optional extension), andperRuleTimingEnabled atomic.BoolRuleGroup.Evalgated on anatomic.Bool; off-state cost is one atomic load per phaseNo breaking changes
coraza.WAFConfigpublic interface: unchangedcoraza.WAFpublic interface: unchangedtypes.RulePhase.String()is additive — the codebase's internalfmtcalls all use%d, so format output for existing callers is unaffectedexperimentalshim pattern (mirrorsWAFConfigWithRuleObserver), so iteration before v4 won't break third-partyWAFConfigimplementersTinyGo
Validated with
tinygo build -target=wasip1. 953 KB wasm, clean build, no new imports outside stdlib in core.Security posture
Documented operator responsibilities (same character as the existing
ErrorLogCbcontract):RuleMatchEvent.Datavalues are attacker-controlled. Sinks MUST NOT use them as metric labels or span attributes — doing so is a cardinality-bomb DoS on the metrics backend. Use them only in structured logs, with redaction and truncation.ErrorLogCb.No detection-evasion path: emits are unconditional at every seam. Sink absence/errors do not mask rule matches. Sink configuration is immutable post-
NewWAFexcept for the per-rule timing atomic flag, which is explicitly concurrent-safe.Tests
experimental/telemetry_sink_test.goexercising every emit seam, Action/Enforced semantics, DetectionOnly mode, body-truncation, runtime toggle, sink-without-extension graceful degradation, nil sink, context propagation, EngineReady, and the third-party WAFConfig/WAF fallback pathsexperimental/telemetry_sink_bench_test.gocoraza.rule.multiphase_evaluationOpportunity for future metrics: OWASP CRS awareness
A small future subpackage — e.g.
coraza/v3/telemetry/crs— could map CRS rule id ranges to category labels (sqli, xss, rce, …) and readtx.anomaly_score_*variables. Sinks opt in by importing; users not on CRS pay nothing. Mentioning this only as a possibility so reviewers understand why the engine stays CRS-agnostic in this PR. Not proposing to ship it.Open questions for reviewers
Is
Action string+Enforced boolonRuleMatchEventthe right split? The existingMatchedRule.Disruptive()conflates "has a disruptive-type action slot" with "will enforce it." I split them because operators asking "how many blocks?" wantAction="deny" && Enforced=true, not a boolean that includespass. Worth pushback.Should
RuleTimingSinkbe a separate interface or a 5th method onTelemetrySink? Chose separate extension so existing sinks stay 4-method and don't force every sink to implement a high-cardinality hook. Engine caches the type assertion atSetTelemetrySinktime so runtime cost is one pointer compare.types.RulePhase.String()— fine or controversial? Added for metric label stability. Internal callers all use%dso no behavior change for them. Any external consumer relying on%voutput would see"request_headers"instead of"1".EngineReadyEventfields — enough, too much, wrong? CurrentlyRulesLoaded+InitDuration. ARulesetHashwould also be useful but means sha-ing all loaded SecLang sources at startup. Skipped pending a reviewer opinion on whether that cost is justified.Should
WithTelemetrySinkandWithPerRuleTimingmove onto the publiccoraza.WAFConfiginterface in v4? Kept them private for now so third-partyWAFConfigimplementations aren't forced to support telemetry.What this PR explicitly does not include
Claude Code (Opus 4.7) was used in the development of this change.
Summary by CodeRabbit
Release Notes