feat(eventrecorder): replace event output schema - #5409
Conversation
Spaceman1701
left a comment
Spaceman1701
left a comment
There was a problem hiding this comment.
I think this change is good in principle, but I don't love that we have to maintain a mapping between v1 and v2 events... I'm generally a little nervous about introducing v2 in general.
How would you feel about changing the v1/v2 selection to be at event construction time? Each New*Event function could pick a v1 or v2 version dynamically. This would at least make it harder to forget about the v1 events when making a change.
| // protobuf) and queues it for asynchronous delivery. It returns the | ||
| // serialized size (for the bytes-written metric). | ||
| func (ko *KafkaOutput) SendEvent(event *eventrecorderpb.Event) (int, error) { | ||
| func (ko *KafkaOutput) SendEvent(event proto.Message) (int, error) { |
There was a problem hiding this comment.
I wonder if there could be a common interface that's more specific than proto.Message to user here. As it is, it makes the API a fair bit less type safe.
There was a problem hiding this comment.
Outputs shouldn't really care about the message contents, that what the proto.Message does here.
Considering you other comment with this one, if we want to make the outputs use a mode specific type, then we have to delay the v1/v2 translation to be requested by the outputs before encoding.
My assumption was that eventrecorder v1 will be deprecated and removed until we reach release v1, then there will be no translation until we have v3, etc.
That is fine with me. |
Alright, that'd be my preference then - I just want to make sure it's easy and safe to add new events. And I'm fine with the idea of deprecating v1 pretty quickly - I doubt it's widely used at this point (though I'm not sure if we're considering it experimental). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (22)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThe event recorder now uses the breaking ChangesEvent recorder v2 migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (5)
eventrecorder/recorder.go (2)
326-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReload-induced drops are indistinguishable from queue-full drops.
Both paths increment
eventsDroppedwith"unknown"/real event types, so operators can't tell a reload window from sustained backpressure. A distinct label value (e.g."config_reload") would keep the signal readable, since the event type genuinely isn't known here.🤖 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 `@eventrecorder/recorder.go` around lines 326 - 331, Update the reload contention path in RecordEvent, specifically the TryRLock failure branch, to increment eventsDropped with the distinct "config_reload" label instead of "unknown". Leave queue-full drop labeling by real event type unchanged.
232-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a labeled break over
gotofor the drain loop.Functionally correct, but a labeled loop expresses the intent without a forward jump.
♻️ Labeled break
- for { - select { - case req := <-c.events: - c.marshalAndSend(req, outputs) - default: - goto drained - } - } - drained: + drain: + for { + select { + case req := <-c.events: + c.marshalAndSend(req, outputs) + default: + break drain + } + }🤖 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 `@eventrecorder/recorder.go` around lines 232 - 240, Replace the forward goto in the event-draining loop within the recorder logic with a labeled break on the enclosing for loop. Preserve the select behavior: continue processing available events from c.events and exit the loop when the default branch is reached, without changing marshalAndSend or the drained continuation.eventrecorder/events.go (2)
409-427: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNil matchers become empty
{}entries in the encoded output.Both
matchersToV1/matchersToV2here and the silence conversions (Lines 463-473 and 488-498) pre-size the slice and leavenilholes, which protobuf/protojson encode as empty messages rather than omitting them. Consumers then see phantom matchers with unspecified type and empty name. Appending only non-nil entries avoids that.♻️ Skip nil entries instead of leaving holes
func matchersToV2(matchers labels.Matchers) []*eventsv2.Matcher { - result := make([]*eventsv2.Matcher, len(matchers)) - for i, matcher := range matchers { - if matcher != nil { - result[i] = &eventsv2.Matcher{Type: matcherTypeToV2(matcher.Type), Name: matcher.Name, Pattern: matcher.Value, Rendered: matcher.String()} - } - } + result := make([]*eventsv2.Matcher, 0, len(matchers)) + for _, matcher := range matchers { + if matcher == nil { + continue + } + result = append(result, &eventsv2.Matcher{Type: matcherTypeToV2(matcher.Type), Name: matcher.Name, Pattern: matcher.Value, Rendered: matcher.String()}) + } return result }🤖 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 `@eventrecorder/events.go` around lines 409 - 427, Update matchersToV1 and matchersToV2 to build result slices by appending only non-nil matchers instead of pre-sizing them and leaving nil holes. Apply the same append-only handling to the referenced silence conversion functions, preserving the existing field mappings for non-nil entries.
345-354: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPanicking on schema mismatch will crash Alertmanager from a producer goroutine.
requireSchemaVersionis reached fromRecordEventon the request/dispatch path, so a single mis-versioned constructor call takes the process down instead of degrading recording. Since the event recorder is an observability side-channel, consider failing safe (log at error level and drop the event) and keeping the panic for tests only.🤖 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 `@eventrecorder/events.go` around lines 345 - 354, Update requireSchemaVersion and its RecordEvent call path to avoid panicking on production schema mismatches: log the mismatch at error level and drop the event instead. Preserve panic behavior only in test-specific validation, if an existing mechanism supports it, and ensure valid schema versions continue recording unchanged.eventrecorder/events_test.go (1)
86-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend nil-matcher coverage to serialization.
Construction is the cheap half; the interesting behavior is what a
nilmatcher/matcher-set produces on the wire (see the nil-hole note ineventrecorder/events.go). Adding aMarshalJSON/MarshalProtobufassertion here would lock the encoded shape.🤖 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 `@eventrecorder/events_test.go` around lines 86 - 92, Extend TestConstructorsHandleNilMatchers to serialize both the nil matcher in the alert group and the nil matcher-set in NewSilenceCreatedEvent, using MarshalJSON and MarshalProtobuf as appropriate. Assert the encoded output matches the documented nil-hole shape in events.go, including preservation of the nil entries rather than silently dropping or panicking.
🤖 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.
Nitpick comments:
In `@eventrecorder/events_test.go`:
- Around line 86-92: Extend TestConstructorsHandleNilMatchers to serialize both
the nil matcher in the alert group and the nil matcher-set in
NewSilenceCreatedEvent, using MarshalJSON and MarshalProtobuf as appropriate.
Assert the encoded output matches the documented nil-hole shape in events.go,
including preservation of the nil entries rather than silently dropping or
panicking.
In `@eventrecorder/events.go`:
- Around line 409-427: Update matchersToV1 and matchersToV2 to build result
slices by appending only non-nil matchers instead of pre-sizing them and leaving
nil holes. Apply the same append-only handling to the referenced silence
conversion functions, preserving the existing field mappings for non-nil
entries.
- Around line 345-354: Update requireSchemaVersion and its RecordEvent call path
to avoid panicking on production schema mismatches: log the mismatch at error
level and drop the event instead. Preserve panic behavior only in test-specific
validation, if an existing mechanism supports it, and ensure valid schema
versions continue recording unchanged.
In `@eventrecorder/recorder.go`:
- Around line 326-331: Update the reload contention path in RecordEvent,
specifically the TryRLock failure branch, to increment eventsDropped with the
distinct "config_reload" label instead of "unknown". Leave queue-full drop
labeling by real event type unchanged.
- Around line 232-240: Replace the forward goto in the event-draining loop
within the recorder logic with a labeled break on the enclosing for loop.
Preserve the select behavior: continue processing available events from c.events
and exit the loop when the default branch is reached, without changing
marshalAndSend or the drained continuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ca68abff-5a35-433b-9f83-90d312cc9f52
⛔ Files ignored due to path filters (1)
eventrecorder/events/v2/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
CHANGELOG.mdapp/app.gobuf.yamlconfig/config_test.godispatch/dispatch.godocs/configuration.mdeventrecorder/config.goeventrecorder/events.goeventrecorder/events_test.goeventrecorder/file.goeventrecorder/kafka.goeventrecorder/kafka_test.goeventrecorder/recorder.goeventrecorder/recorder_test.goeventrecorder/stdout.goeventrecorder/webhook.goeventrecorder/webhook_test.goinhibit/inhibit.gonotify/event.gonotify/retry_stage.goproto/eventrecorder/events/v2/events.protoprovider/mem/mem.gosilence/silence.go
8f4037c to
c24de91
Compare
siavashs
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@eventrecorder/events.go`:
- Around line 291-310: Extend the v2 events.Silence contract with a
receiver_matcher_sets field, regenerate its bindings, and update silenceToEvents
to snapshot silence.ReceiverMatcherSets using the same conversion and
nil-preservation behavior as MatcherSets. Add constructor coverage confirming
receiver-scoped matcher sets are retained in the recorded silence.
- Around line 302-305: Update the conversion logic around the matchers variable
to derive events.Silence.Matchers directly from silence.Matchers when legacy
matchers are present, and use silence.MatcherSets[0].Matchers only when the
legacy field is absent or empty. Add a regression test covering a silence with
legacy Matchers and no MatcherSets, ensuring the converted silence preserves its
selectors.
- Around line 331-345: The silenceMatcherRendered function currently defaults
unknown silencepb.Matcher types to labels.MatchEqual; instead, explicitly accept
only the four supported enum values and return an empty rendered value for any
unknown or unspecified type before calling labels.NewMatcher. Add a test
covering an unknown enum value and asserting an empty result.
In `@eventrecorder/recorder.go`:
- Around line 311-315: Update the clusterPosition handling in the event metadata
construction to avoid converting peer.Position() directly to uint32; use uint or
the protobuf field’s uint representation so the full cluster-size range is
preserved. Keep the nil-peer default at zero and continue passing
clusterPosition to event.withMetadata.
In `@notify/event.go`:
- Around line 106-111: The exported functions NewAlertResolvedEvent and
NewAlertGroupedEvent are missing documentation comments required by Go coding
standards. Add a full-sentence comment (ending with a period) above each
function that describes what it does. NewAlertResolvedEvent should document that
it creates an alert-resolved event, and NewAlertGroupedEvent should document
that it creates an alert-grouped event.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c06043-497f-467e-894e-7c3cbfa6efaa
⛔ Files ignored due to path filters (2)
eventrecorder/eventrecorderpb/eventrecorder.pb.gois excluded by!**/*.pb.goeventrecorder/events/v2/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
CHANGELOG.mdapp/app.gobuf.yamldispatch/dispatch.godocs/configuration.mdeventrecorder/eventrecorderpb/eventrecorder.protoeventrecorder/events.goeventrecorder/events_test.goeventrecorder/file.goeventrecorder/kafka.goeventrecorder/kafka_test.goeventrecorder/recorder.goeventrecorder/recorder_test.goeventrecorder/stdout.goeventrecorder/webhook.goeventrecorder/webhook_test.goinhibit/inhibit.gonotify/event.gonotify/retry_stage.goproto/eventrecorder/events/v2/events.protoprovider/mem/mem.gosilence/silence.go
💤 Files with no reviewable changes (1)
- eventrecorder/eventrecorderpb/eventrecorder.proto
🚧 Files skipped from review as they are similar to previous changes (13)
- CHANGELOG.md
- app/app.go
- docs/configuration.md
- eventrecorder/file.go
- silence/silence.go
- eventrecorder/kafka_test.go
- notify/retry_stage.go
- eventrecorder/kafka.go
- provider/mem/mem.go
- eventrecorder/webhook_test.go
- eventrecorder/events_test.go
- eventrecorder/stdout.go
- proto/eventrecorder/events/v2/events.proto
Replace the original event recorder schema with events/v2. Alert labels, alert annotations, group labels, silence annotations, and muted-alert labels are now encoded as maps instead of nested ordered label pairs. This is a breaking change for every event recorder output. JSON consumers must handle the new map-based fields, and protobuf consumers must regenerate their bindings from proto/eventrecorder/events/v2/events.proto. Remove the legacy eventrecorderpb schema and generated bindings. Register the new schema as the event recorder Buf module and use it directly for all file, webhook, Kafka, and stdout outputs without version selection or conversion. Event producers now construct opaque eventrecorder.Event values through snapshotting constructors instead of depending on protobuf types. Destinations receive the typed Event and serialize it as JSON or protobuf. Event metadata is attached without mutating the constructed payload. Also include silence annotations in recorded events. BREAKING CHANGE: Event recorder outputs now use the events/v2 schema and map-based label and annotation fields. The legacy eventrecorderpb wire format is no longer supported. Signed-off-by: Siavash Safi <siavash@cloudflare.com>
c24de91 to
3caba4c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3 participants
Replace the original event recorder schema with events/v2. Alert labels,
alert annotations, group labels, silence annotations, and muted-alert labels
are now encoded as maps instead of nested ordered label pairs.
This is a breaking change for every event recorder output. JSON consumers must
handle the new map-based fields, and protobuf consumers must regenerate their
bindings from proto/eventrecorder/events/v2/events.proto.
Remove the legacy eventrecorderpb schema and generated bindings. Register the
new schema as the event recorder Buf module and use it directly for all file,
webhook, Kafka, and stdout outputs without version selection or conversion.
Event producers now construct opaque eventrecorder.Event values through
snapshotting constructors instead of depending on protobuf types. Destinations
receive the typed Event and serialize it as JSON or protobuf. Event metadata is
attached without mutating the constructed payload.
Also include silence annotations in recorded events.
BREAKING CHANGE: Event recorder outputs now use the events/v2 schema and
map-based label and annotation fields. The legacy eventrecorderpb wire format
is no longer supported.
Signed-off-by: Siavash Safi siavash@cloudflare.com