Add configurable global default check output size

[elfranne]

What is this change?

Adds a configurable, cluster-wide default check output size (--event-default-max-output-size) that eventd applies to incoming events whose check does not set its own max_output_size.

Why is this change necessary?

Checks that produce very large output can generate events too big for etcd's request size limit (~1.5 MiB), causing the store to reject the event. Today the only mitigation is setting max_output_size on every individual check, which is easy to forget. This gives operators a single global backstop: any event without a per-check limit gets a sane default (1.4 MiB, kept ~100 KiB below etcd's limit for headroom), so the store truncates oversized output — recording how many bytes were dropped — instead of rejecting the event outright. Per-check max_output_size still takes precedence, so existing behavior is unchanged for checks that already set it.

Does your change need a Changelog entry?

Yes. Suggested entry under Added:

Added a configurable global default check output size via the --event-default-max-output-size backend flag. Events whose check does not set its own max_output_size fall back to this default (1.4 MiB by default; 0 disables it).

Do you need clarification on anything?

One point worth a reviewer's eye:

• The default value (1468006 bytes, ~1.4 MiB) is derived from etcd's 1.5 MiB DefaultMaxRequestBytes minus ~100 KiB of headroom for the rest of the marshaled event and gRPC framing. An event without output is only ~5-6 KiB, so the ~100 KiB margin is comfortably larger than the non-output portion of a typical event.
• The default is applied in eventd.handleMessage before the event is passed to the store; the store still performs the actual truncation based on Check.MaxOutputSize.

Were there any complications while making this change?

No significant complications. The bulk of the diff in eventd.go is gofmt re-aligning the struct field columns after adding the new defaultMaxOutputSize field.

Have you reviewed and updated the documentation for this change? Is new documentation required?

The new --event-default-max-output-size flag will need a mention in the backend configuration reference documentation. A docs PR should be filed and linked here.

How did you verify this change?

Added TestDefaultMaxOutputSize in backend/eventd/eventd_test.go, covering:

• the global default is applied when the check does not set max_output_size
• a per-check max_output_size takes precedence over the default
• with no default and no per-check value, max_output_size is left unset (0)

The test asserts on the Check.MaxOutputSize of the event handed to the store's UpdateEvent; the actual output truncation is performed by the store and is covered by the existing etcd store tests.

Is this change a patch?

No — this is additive feature work and targets [main](sensu:develop/6).

[sourabhpatel-sumo]

Tested this locally against a real embedded etcd (not mocks). Confirmed the bug on develop/6 — a check event with no max_output_size and >1.5 MiB output gets flat-out rejected: internal error: etcdserver: request is too large

Event's gone, check result never makes it to the store. With this PR applied, same scenario now succeeds — Check.MaxOutputSize gets the default stamped on it, output is truncated, and sensu.io/output_truncated_bytes shows up correctly. Ran the full backend/eventd suite including -tags=integration, all green.

@elfranne One thing I'd fix before merging: TestDefaultMaxOutputSize only asserts against a mocked UpdateEvent, so it proves the field gets set but never actually proves etcd stops rejecting the write — which is the whole point of this change. Would be worth adding an integration test (there's already a good pattern for this in TestEventdMonitor) that spins up real etcd and sends an oversized check output through, so we have proof it actually fixes the rejection and not just the struct field.

Otherwise this looks solid — small, focused change, and per-check max_output_size still wins like it should.

[sourabhpatel-sumo]

@elfranne Wrote the integration test I mentioned above — drops straight into backend/eventd/integration_test.go next to TestEventdMonitor, same real-embedded-etcd pattern.

diff --git a/backend/eventd/integration_test.go b/backend/eventd/integration_test.go
index 373ef69e2..59158b2b3 100644
--- a/backend/eventd/integration_test.go
+++ b/backend/eventd/integration_test.go
@@ -5,6 +5,7 @@ package eventd

 import (
      "context"
+     "strings"
      "testing"

      corev2 "github.com/sensu/core/v2"
@@ -106,3 +107,41 @@ func TestEventdMonitor(t *testing.T) {
      assert.NoError(t, sub.Cancel())
      close(eventChan)
 }
+
+// TestEventdDefaultMaxOutputSize verifies against a real embedded etcd that
+// an event whose check produces output larger than etcd's request-size
+// limit, and does not set its own MaxOutputSize, is truncated to the
+// configured default and stored — instead of being rejected by etcd.
+func TestEventdDefaultMaxOutputSize(t *testing.T) {
+     store, err := testutil.NewStoreInstance()
+     require.NoError(t, err)
+     defer store.Teardown()
+
+     require.NoError(t, seeds.SeedInitialData(store))
+
+     storev2 := etcdstore.NewStore(store.Client)
+
+     bus, err := messaging.NewWizardBus(messaging.WizardBusConfig{})
+     require.NoError(t, err)
+     require.NoError(t, bus.Start())
+
+     e := newEventd(storev2, store, bus, newFakeFactory(&fakeSwitchSet{}))
+     e.defaultMaxOutputSize = 1468006 // matches the PR's default flag value
+
+     event := corev2.FixtureEvent("entity1", "check1")
+     // Leave MaxOutputSize unset (zero value) — the common case the defaul
+     event.Check.Output = strings.Repeat("x", 2*1024*1024) // 2 MiB, over etcd's 1.5 MiB request limit
+
+     ctx := otherTestutil.ContextWithNamespace("default")(context.Background())
+     require.NoError(t, store.UpdateEntity(ctx, event.Entity))
+
+     _, err = e.handleMessage(event)
+     require.NoError(t, err, "expected the default MaxOutputSize to prevent etcd rejection")
+
+     stored, err := store.GetEventByEntityCheck(ctx, "entity1", "check1")
+     require.NoError(t, err)
+     require.NotNil(t, stored)
+     require.Equal(t, int64(1468006), stored.Check.MaxOutputSize)
+     require.Len(t, stored.Check.Output, 1468006)
+     require.Equal(t, "629146", stored.Labels["sensu.io/output_truncated_by68006
+}

[elfranne]

added your integration test with minor changes:

• require.Equal(t, "629146", stored.Labels["sensu.io/output_truncated_bytes"])
• comment on L129

[sourabhpatel-sumo]

Thanks for adding the integration test — I traced it through handleMessage → updateEventWithDuration → etcd.(*Store).UpdateEvent and the assertions all hold, including the 629146 label value. Few things I'd like before merge:

• Can we also add Rejection assertion like this, then we will have a real regression test for bug -

  event.Check.MaxOutputSize = 0
  e.defaultMaxOutputSize = 0
  _, err = e.handleMessage(event)
  require.ErrorContains(t, err, "request is too large")

• Truncation is silent — please add a warn log.

  Right now the only trace of dropped output is the sensu.io/output_truncated_bytes label, which nobody finds unless they already suspect output loss. Best place is the new block in handleMessage, since it knows both the output length and the limit it's about to apply — so it logs only on actual data loss, and only for the new code path (per-check max_output_size behavior stays untouched):
  glogger.EventFields(event, false) is already the convention in this file (see eventd.go:529), so namespace/entity/check come along for free.

  if e.defaultMaxOutputSize > 0 && event.Check.MaxOutputSize <= 0 {
        event.Check.MaxOutputSize = e.defaultMaxOutputSize
        if outputSize := int64(len(event.Check.Output)); outputSize > e.defaultMaxOutputSize {
                logger.WithFields(glogger.EventFields(event, false)).
                        WithField("truncated_bytes", outputSize-e.defaultMaxOutputSize).
                        Warn("check output exceeds the global default max output size and will be truncated")
        }
  }

  One caveat: a check that always overflows will log on every interval, per entity — and log volume is a live concern here (#5171 just added size-based rotation). Warn is still right for silent data loss, but consider pairing it with a counter so it's alertable without log scraping; event_store.go:41-46 has the registration pattern for a sensu_go_store_event_output_truncated_total.

• 1468006 is hardcoded in four places (start.go plus three times in the test), and worse, it's derived from a limit the operator can already change.

  defaultEventMaxOutputSize is a hardcoded constant computed from the default value of --etcd-max-request-bytes — but that's an existing, documented flag (start.go:114, :559). The two are coupled by an invariant with nothing enforcing it:

  • Operator lowers --etcd-max-request-bytes below 1468006 → the new default provides zero protection, events are still rejected with request is too large, and the feature silently doesn't do the one thing it was added for.
  • Operator raises it → 1.4 MiB is needlessly conservative, output keeps getting truncated that etcd would now accept, with no indication why.

  Rather than hardcoding the constant (and retyping it in the test), derive it. Initialize in backend/backend.go already has both values in hand — it reads config.EtcdMaxRequestBytes at :281 and builds eventd.Config at :492:

  const eventOutputHeadroom = 100 << 10 // 100 KiB for the rest of the marshaled event + gRPC framing
  
  maxRequestBytes := config.EtcdMaxRequestBytes
  if maxRequestBytes == 0 {
        maxRequestBytes = etcd.DefaultMaxRequestBytes
  }
  
  defaultMaxOutputSize := config.EventDefaultMaxOutputSize
  if defaultMaxOutputSize == 0 {
        defaultMaxOutputSize = int64(maxRequestBytes) - eventOutputHeadroom
  } else if defaultMaxOutputSize >= int64(maxRequestBytes) {
        return nil, fmt.Errorf(
                "--event-default-max-output-size (%d) must be below --etcd-max-request-bytes (%d)",
                defaultMaxOutputSize, maxRequestBytes)
  }

  Note this shifts the default from 1468006 → 1470464. The existing value isn't actually "1.5 MiB minus 100 KiB" as the comment claims — it's just 1.4 * 1048576 truncated, i.e. ~102.4 KiB of headroom. Deriving it makes the stated relationship real instead of approximate.

  This kills the magic number and the coupling in one move: the flag default becomes "track the etcd limit," an explicit override that can't work is rejected at startup instead of failing per-event at runtime, and the 0 = disabled semantics need a distinct sentinel (or a separate bool) since 0 currently means both "disabled" and "unset."

  For the test, export whatever the final resting place is so integration_test.go can reference it instead of retyping 1468006 three times, and derive the label expectation as fmt.Sprint(outputSize - <thatValue>) rather than hardcoding "629146".

• Please add a CHANGELOG-6.md entry under Added — this introduces a user-facing backend flag, and the PR description already flags one as needed.

[sourabhpatel-sumo]

Heads-up on scope, not a new review item — the four above stand as written.

#5103 (check output in check history) is accepted as an opt-in feature, and its per-entry size budget has to be derived from --etcd-max-request-bytes for exactly the reason in my third comment: history multiplies output 21× against a ceiling that max_output_size can't see, so the budget has to track whatever the operator actually configured.

That means the derive-don't-hardcode change requested above is now load-bearing for two features rather than one. If 1468006 stays a constant, #5103 either duplicates the same magic number or inherits the same silent-failure mode when an operator retunes --etcd-max-request-bytes.

No change to what's being asked here — just extra reason to land the derivation in backend/backend.go where both features can consume it. Happy to take these items over if you'd rather not carry them.

[elfranne]

@sourabhpatel-sumo apologies, the follow-up commit had been sitting unpushed on my side. It's up now as c78e023 and covers all four review items plus the two inline comments:

• Rejection assertion: TestEventdDefaultMaxOutputSize now also sends the same 2 MiB output with the default disabled and asserts request is too large. It uses a fresh event on check2, because handleMessage mutates the event in place (MaxOutputSize and the truncation label).
• Silent truncation: added a warn log in the new handleMessage block, using glogger.EventFields(event, false) plus truncated_bytes. Also added a sensu_go_eventd_output_truncated_total counter so it's alertable. Both fire only on actual data loss and only on the new code path.
• Derived, not hardcoded: 1468006 is gone. backend.Initialize (backend/backend.go) resolves the default from --etcd-max-request-bytes (falling back to etcd.DefaultMaxRequestBytes) minus 100 KiB of headroom, so the default moves to 1470464 as you noted. An explicit value at or above the etcd limit is rejected at startup. The flag default is now -1 ("derive"), which separates "unset" from 0 ("disabled"). The test derives both the size and the label value via the exported eventd.DeriveMaxOutputSize instead of retyping constants.
• CHANGELOG-6.md: entry added under Added.
• Inline: fixed the doc-comment name on defaultEventMaxOutputSize, and dropped the store annotation so the flag sits under General Flags with the other --event-* flags.

Locally, go test ./backend/eventd/... ./backend/cmd/... ./backend/ and -tags=integration for TestEventdDefaultMaxOutputSize / TestEventdMonitor are all green.

On #5103: the resolution against the operator's configured --etcd-max-request-bytes already happens in backend.Initialize, so check history can consume it there rather than duplicating the number. The helper is eventd.DeriveMaxOutputSize / eventd.OutputSizeHeadroom. If #5103 doesn't live in eventd and you'd rather not depend on it from there, I'm happy to move the helper next to DefaultMaxRequestBytes in backend/etcd. Just say which you prefer.

Thanks for the offer to take these over. No need, it's all in now.

[sourabhpatel-sumo]

@elfranne Verified changed and all four review items are addressed correctly ✨.

For #5103, I would like it to move the helper next to DefaultMaxRequestBytes in backend/etcd, reason being check-output-history work doesn't touch eventd at all — check history is built in backend/store/etcd/event_store.go, a storage-layer package. backend/store/etcd already imports backend/etcd (e.g. initialization.go, silenced_store.go), so there's no cycle risk there. Having it depend on backend/eventd instead would be a layering inversion — store code reaching into a daemon package — and only works today because nothing imports eventd back.

Also, requesting @chavakula to verify and check if we overlooked something.