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). |
📝 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
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>
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
| matcherSets := make([]*events.MatcherSet, len(silence.MatcherSets)) | ||
| for i, set := range silence.MatcherSets { | ||
| if set == nil { | ||
| continue | ||
| } | ||
| for j, m := range ms.Matchers { | ||
| matcherSet.Matchers[j] = SilenceMatcherAsProto(m) | ||
| matchers := make([]*events.Matcher, len(set.Matchers)) | ||
| for j, matcher := range set.Matchers { | ||
| matchers[j] = silenceMatcherToEvents(matcher) | ||
| } | ||
| matcherSets[i] = matcherSet | ||
| matcherSets[i] = &events.MatcherSet{Matchers: matchers} | ||
| } | ||
|
|
||
| var matchers []*eventrecorderpb.Matcher | ||
| if len(matcherSets) > 0 { | ||
| var matchers []*events.Matcher | ||
| if len(matcherSets) > 0 && matcherSets[0] != nil { | ||
| matchers = matcherSets[0].Matchers | ||
| } | ||
|
|
||
| return &eventrecorderpb.Silence{ | ||
| Id: sil.Id, | ||
| Matchers: matchers, | ||
| MatcherSets: matcherSets, | ||
| StartsAt: sil.StartsAt, | ||
| EndsAt: sil.EndsAt, | ||
| UpdatedAt: sil.UpdatedAt, | ||
| CreatedBy: sil.CreatedBy, | ||
| Comment: sil.Comment, | ||
| return &events.Silence{ | ||
| Id: silence.Id, Matchers: matchers, Annotations: stringMap(silence.Annotations), StartsAt: cloneTimestamp(silence.StartsAt), | ||
| EndsAt: cloneTimestamp(silence.EndsAt), UpdatedAt: cloneTimestamp(silence.UpdatedAt), CreatedBy: silence.CreatedBy, | ||
| Comment: silence.Comment, MatcherSets: matcherSets, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Record receiver matcher sets.
silenceToEvents snapshots MatcherSets but drops silence.ReceiverMatcherSets. The v2 events.Silence contract also has no receiver-matcher-set field. A receiver-scoped silence is recorded without its receiver restriction.
Add receiver_matcher_sets to the v2 proto, regenerate bindings, snapshot the field here, and add constructor coverage.
🤖 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 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.
| var matchers []*events.Matcher | ||
| if len(matcherSets) > 0 && matcherSets[0] != nil { | ||
| matchers = matcherSets[0].Matchers | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve legacy Silence.Matchers values.
At Line 302, the converter derives events.Silence.Matchers only from silence.MatcherSets[0]. A valid silencepb.Silence can contain legacy Matchers with no MatcherSets. This path records no selector for that silence.
Convert silence.Matchers directly. Use the first matcher set only as a fallback. Add a regression test for legacy matchers without matcher sets.
Proposed conversion
var matchers []*events.Matcher
- if len(matcherSets) > 0 && matcherSets[0] != nil {
+ if len(silence.Matchers) > 0 {
+ matchers = make([]*events.Matcher, len(silence.Matchers))
+ for i, matcher := range silence.Matchers {
+ matchers[i] = silenceMatcherToEvents(matcher)
+ }
+ } else if len(matcherSets) > 0 && matcherSets[0] != nil {
matchers = matcherSets[0].Matchers
}📝 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.
| var matchers []*events.Matcher | |
| if len(matcherSets) > 0 && matcherSets[0] != nil { | |
| matchers = matcherSets[0].Matchers | |
| } | |
| var matchers []*events.Matcher | |
| if len(silence.Matchers) > 0 { | |
| matchers = make([]*events.Matcher, len(silence.Matchers)) | |
| for i, matcher := range silence.Matchers { | |
| matchers[i] = silenceMatcherToEvents(matcher) | |
| } | |
| } else if len(matcherSets) > 0 && matcherSets[0] != nil { | |
| matchers = matcherSets[0].Matchers | |
| } |
🤖 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 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.
| func silenceMatcherRendered(matcher *silencepb.Matcher) string { | ||
| matcherType := labels.MatchEqual | ||
| switch matcher.Type { | ||
| case silencepb.Matcher_REGEXP: | ||
| matcherType = labels.MatchRegexp | ||
| case silencepb.Matcher_NOT_EQUAL: | ||
| matcherType = labels.MatchNotEqual | ||
| case silencepb.Matcher_NOT_REGEXP: | ||
| matcherType = labels.MatchNotRegexp | ||
| } | ||
| } | ||
|
|
||
| // NewSilenceCreatedEvent constructs a SilenceCreated event. | ||
| func NewSilenceCreatedEvent(silence *eventrecorderpb.Silence) *eventrecorderpb.EventData { | ||
| return &eventrecorderpb.EventData{ | ||
| EventType: &eventrecorderpb.EventData_SilenceCreated{ | ||
| SilenceCreated: &eventrecorderpb.SilenceCreatedEvent{ | ||
| Silence: silence, | ||
| }, | ||
| }, | ||
| rendered := "" | ||
| if parsed, err := labels.NewMatcher(matcherType, matcher.Name, matcher.Pattern); err == nil { | ||
| rendered = parsed.String() | ||
| } | ||
| return rendered |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not render an unknown matcher type as equality.
At Line 332, the default matcher type is equality. For an unknown source enum, Line 328 emits TYPE_UNSPECIFIED but Line 343 emits an equality-form rendered matcher.
Return an empty rendered value for unknown types. Add a test with an unknown enum value.
Proposed fix
func silenceMatcherRendered(matcher *silencepb.Matcher) string {
- matcherType := labels.MatchEqual
+ var matcherType labels.MatchType
switch matcher.Type {
+ case silencepb.Matcher_EQUAL:
+ matcherType = labels.MatchEqual
case silencepb.Matcher_REGEXP:
matcherType = labels.MatchRegexp
case silencepb.Matcher_NOT_EQUAL:
matcherType = labels.MatchNotEqual
case silencepb.Matcher_NOT_REGEXP:
matcherType = labels.MatchNotRegexp
+ default:
+ return ""
}📝 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 silenceMatcherRendered(matcher *silencepb.Matcher) string { | |
| matcherType := labels.MatchEqual | |
| switch matcher.Type { | |
| case silencepb.Matcher_REGEXP: | |
| matcherType = labels.MatchRegexp | |
| case silencepb.Matcher_NOT_EQUAL: | |
| matcherType = labels.MatchNotEqual | |
| case silencepb.Matcher_NOT_REGEXP: | |
| matcherType = labels.MatchNotRegexp | |
| } | |
| } | |
| // NewSilenceCreatedEvent constructs a SilenceCreated event. | |
| func NewSilenceCreatedEvent(silence *eventrecorderpb.Silence) *eventrecorderpb.EventData { | |
| return &eventrecorderpb.EventData{ | |
| EventType: &eventrecorderpb.EventData_SilenceCreated{ | |
| SilenceCreated: &eventrecorderpb.SilenceCreatedEvent{ | |
| Silence: silence, | |
| }, | |
| }, | |
| rendered := "" | |
| if parsed, err := labels.NewMatcher(matcherType, matcher.Name, matcher.Pattern); err == nil { | |
| rendered = parsed.String() | |
| } | |
| return rendered | |
| func silenceMatcherRendered(matcher *silencepb.Matcher) string { | |
| var matcherType labels.MatchType | |
| switch matcher.Type { | |
| case silencepb.Matcher_EQUAL: | |
| matcherType = labels.MatchEqual | |
| case silencepb.Matcher_REGEXP: | |
| matcherType = labels.MatchRegexp | |
| case silencepb.Matcher_NOT_EQUAL: | |
| matcherType = labels.MatchNotEqual | |
| case silencepb.Matcher_NOT_REGEXP: | |
| matcherType = labels.MatchNotRegexp | |
| default: | |
| return "" | |
| } | |
| rendered := "" | |
| if parsed, err := labels.NewMatcher(matcherType, matcher.Name, matcher.Pattern); err == nil { | |
| rendered = parsed.String() | |
| } | |
| return rendered | |
| } |
🤖 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 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.
| clusterPosition := uint32(0) | ||
| if peer := r.core.peer.Load(); peer != nil { | ||
| wrappedEvent.ClusterPosition = uint32(peer.Position()) | ||
| clusterPosition = uint32(peer.Position()) | ||
| } | ||
| event = event.withMetadata(timestamppb.Now(), r.core.instance, clusterPosition) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the cluster API before inspecting the Position implementation.
ast-grep outline cluster --items all --type method,function --match Position
# Inspect the return type, assignments, and range controls for Peer.Position.
rg -nP -C 8 'func\s+\([^)]*\bPeer\b[^)]*\)\s+Position\s*\(' cluster --glob '*.go'
rg -nP -C 5 '\bposition\s*=' cluster --glob '*.go'
# Confirm the receiving protobuf field type and all local uses.
rg -n -C 3 'cluster_position|ClusterPosition' proto/eventrecorder eventrecorder \
--glob '*.proto' --glob '*.go'Repository: prometheus/alertmanager
Length of output: 924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cluster/cluster.go Position implementation =="
sed -n '650,675p' cluster/cluster.go
echo
echo "== cluster/cluster.go relevant position/index declarations =="
rg -n -C 4 '\bIndex\b|position|Position\s*\(' cluster/cluster.go
echo
echo "== eventrecorder/proto cluster_position usage =="
rg -n -C 4 'cluster_position|ClusterPosition|cluster_position ' proto eventrecorder --glob '*.proto' --glob '*.go' || true
echo
echo "== Go semantic simulation of out-of-range uint32 conversion =="
python3 - <<'PY'
for v in [-1, 0, 4294967294, 4294967295, 4294967296, 9223372036854775807, -2**63]:
def to_uint32(v):
return v & ((1 << 32) - 1)
print(f"{v}: int(v)%2**32={v%2**32}, conversion-like={to_uint32(v)}")
PYRepository: prometheus/alertmanager
Length of output: 5282
Remove the uint32 conversion for Peer.Position.
Peer.Position() returns the zero-based index in the sorted member slice. Since all is only returned by Peers(), the result is bounded by the current cluster size, not a generic int. Cast through uint or store the value as a uint in the protobuf message to preserve the full cluster-size range and avoid the non-portable signed-to-unsigned conversion.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 312-312: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(peer.Position())
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 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.
Source: Linters/SAST tools
| func NewAlertResolvedEvent(groupInfo eventrecorder.AlertGroup, alert *types.Alert) eventrecorder.Event { | ||
| return eventrecorder.NewAlertResolvedEvent(groupInfo, groupedAlertEvent(alert)) | ||
| } | ||
|
|
||
| func NewAlertGroupedEvent(groupInfo *eventrecorderpb.AlertGroupInfo, alert *types.Alert) *eventrecorderpb.EventData { | ||
| return &eventrecorderpb.EventData{ | ||
| EventType: &eventrecorderpb.EventData_AlertGrouped{ | ||
| AlertGrouped: &eventrecorderpb.AlertGroupedEvent{ | ||
| Alert: groupedAlertAsProto(alert), | ||
| GroupInfo: groupInfo, | ||
| }, | ||
| }, | ||
| } | ||
| func NewAlertGroupedEvent(groupInfo eventrecorder.AlertGroup, alert *types.Alert) eventrecorder.Event { | ||
| return eventrecorder.NewAlertGroupedEvent(groupInfo, groupedAlertEvent(alert)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add comments for the exported constructors.
NewAlertResolvedEvent and NewAlertGroupedEvent have no exported-identifier comments. Add a full sentence for each function.
As per coding guidelines, “Comments on exported Go identifiers must be full sentences ending with a period.”
🤖 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 `@notify/event.go` around lines 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.
Source: Coding guidelines
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