Skip to content

Commit 5fd1805

Browse files
committed
chore(review): address feedback for eventlogger queue log, chimux dynamic route comment, eventbus exporter guidance, rotation guard cleanup, shutdown drain semantics
1 parent a9f123e commit 5fd1805

5 files changed

Lines changed: 19 additions & 10 deletions

File tree

modules/chimux/module.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -860,7 +860,12 @@ func (m *ChiMuxModule) disabledRouteMiddleware() func(http.Handler) http.Handler
860860
if rctx != nil && len(rctx.RoutePatterns) > 0 {
861861
pattern = rctx.RoutePatterns[len(rctx.RoutePatterns)-1]
862862
} else {
863-
// Fallback to request path (may cause mismatch for dynamic patterns)
863+
// Fallback to the raw request path. WARNING: For parameterized routes (e.g. /users/{id})
864+
// chi records the pattern as /users/{id} but r.URL.Path will be the concrete value
865+
// such as /users/123. This means a disabled route registered as /users/{id} will NOT
866+
// match here and the route may remain active. Admin tooling disabling dynamic routes
867+
// should therefore prefer invoking DisableRoute() with the original pattern captured
868+
// at registration time rather than a concrete request path.
864869
pattern = r.URL.Path
865870
}
866871
method := r.Method

modules/eventbus/memory.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -218,10 +218,7 @@ func (m *MemoryEventBus) Publish(ctx context.Context, event Event) error {
218218
// (when rotation disabled) to preserve deterministic ordering and avoid per-publish RNG cost.
219219
if m.config.RotateSubscriberOrder && len(allMatchingSubs) > 1 {
220220
pc := atomic.AddUint64(&m.pubCounter, 1) - 1
221-
ln := len(allMatchingSubs)
222-
if ln <= 0 {
223-
return nil
224-
}
221+
ln := len(allMatchingSubs) // ln >= 2 here due to enclosing condition
225222
// Compute rotation starting offset. We keep start as uint64 and avoid any uint64->int cast
226223
// (gosec G115) by performing a manual copy instead of slicing with an int index.
227224
start64 := pc % uint64(ln)

modules/eventbus/metrics_exporters.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,10 @@ package eventbus
2121
// go exporter.Run(ctx)
2222
// ... later cancel();
2323
//
24-
// NOTE: Prometheus and Datadog dependencies are optional; if removed, comment out related code.
24+
// NOTE: Prometheus and Datadog dependencies are optional. If you want to exclude one of these
25+
// exporters for a build, prefer Go build tags (e.g. //go:build !prometheus) with the exporter
26+
// implementation moved to a separate file guarded by that tag, rather than manual comment edits.
27+
// This file keeps both implementations active by default for convenience.
2528

2629
import (
2730
"context"

modules/eventlogger/config.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ type EventLoggerConfig struct {
4242
ShutdownEmitStopped bool `yaml:"shutdownEmitStopped" default:"true" desc:"Emit logger stopped operational event on Stop"`
4343

4444
// ShutdownDrainTimeout specifies how long Stop() should wait for in-flight events to drain.
45-
// A zero or negative duration means unlimited wait (current behavior using WaitGroup).
46-
ShutdownDrainTimeout time.Duration `yaml:"shutdownDrainTimeout" default:"2s" desc:"Maximum time to wait for draining event queue on Stop"`
45+
// Zero or negative duration means unlimited wait (Stop blocks until all events processed).
46+
ShutdownDrainTimeout time.Duration `yaml:"shutdownDrainTimeout" default:"2s" desc:"Maximum time to wait for draining event queue on Stop. Zero or negative = unlimited wait."`
4747
}
4848

4949
// OutputTargetConfig configures a specific output target for event logs.

modules/eventlogger/module.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -604,14 +604,18 @@ func (m *EventLoggerModule) OnEvent(ctx context.Context, event cloudevents.Event
604604
return
605605
} else {
606606
// Queue is full - drop oldest event and add new one
607+
var droppedEventType string
607608
if len(m.eventQueue) > 0 {
608-
// Shift slice to remove first element (oldest)
609+
// Capture dropped event type for debugging visibility then shift slice
610+
droppedEventType = m.eventQueue[0].Type()
609611
copy(m.eventQueue, m.eventQueue[1:])
610612
m.eventQueue[len(m.eventQueue)-1] = event
611613
}
612614
if m.logger != nil {
613615
m.logger.Debug("Event queue full, dropped oldest event",
614-
"queue_size", m.queueMaxSize, "new_event", event.Type())
616+
"queue_size", m.queueMaxSize,
617+
"new_event", event.Type(),
618+
"dropped_event", droppedEventType)
615619
}
616620
queueResult = nil
617621
return

0 commit comments

Comments
 (0)