[Areson]

sink.Make parses the retry options buffer-size, send-timeout, and send-retry-wait into RetryArgs, then passes the full options map to the specific sink constructor. The chronosphere, signalfx, datadog, and prom-pushgateway sinks all return invalid option: <key> for keys they don't know, so a monitor that sets any retry option can't build its sink.

This PR changes Make to parse the three options in one loop, in a fixed order, so the same invalid options always return the same error. The specific sink and HTTPClient.MakeForSink get a clone of the options with the retry keys deleted. The caller's map isn't modified, and defaults stay the same.

It also validates the retry options:

• buffer-size must be from 1 to 10000 (MAX_RETRY_BUFFER_SIZE). The upper bound stops a typo from allocating a huge buffer slice. 10000 is about 2.8 hours of batches at a 1s interval.
• send-timeout must be greater than zero and at most 1m (MAX_RETRY_SEND_TIMEOUT). The level collector sends to a monitor's sinks one at a time with no timeout of its own, so a long send-timeout would stall the monitor's other sinks during an outage. A negative send-timeout made Retry buffer without ever sending.
• send-retry-wait must be greater than zero. A zero for either duration silently meant the default.

Parse and range errors name the sink and option, for example invalid datadog sink option send-timeout: .... Because a built-in sink used to reject any retry option, no config that loaded before now fails.

The upper bounds and the negative checks live in the new exported RetryArgs.Validate, which the factory calls. NewRetry doesn't call it, so existing Go callers that pass values outside these ranges keep working as before. Custom sinks that build RetryArgs from user input should call Validate first.

Retry.Send now stops waiting send-retry-wait between sends when send-timeout expires. Previously a send-retry-wait longer than send-timeout let a send cycle run past the timeout by a whole wait.

The retry sink docs and the config quick reference showed a separate sinks.retry section, which Make rejects as an unregistered sink. Both now say to set the retry options on each built-in sink. The retry docs list the three options and their valid values, and each built-in sink page links to them.

Testing

• TestFactoryMake_RetryOptions builds each built-in sink through the factory with all three retry options set. It checks that:

  • the parsed values reach the Retry wrapper
  • MakeForSink gets only the sink-specific options
  • the sink registers its sink-retry-dropped counter

  On main it fails for all four sinks with invalid option.

• TestFactoryMake_RetryDefaults checks that the defaults don't change when no retry options are set.

• TestFactoryMake_InvalidRetryOptions checks that non-numeric, zero, negative, and too-large values, and bad durations, are rejected with an error that names the sink and option. It also checks that the maximum values are accepted, and that with several invalid options the error always names the first in parse order.

• TestRetryArgsValidate checks the ranges and that NewRetry still accepts out-of-range values without panicking.

• TestRetry_SendTimeoutBoundsRetryWait checks that a 1m send-retry-wait doesn't hold Send past a 200ms send-timeout. Without the fix, it fails.

• go vet ./..., go test ./..., and go test -race ./sink/... pass.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/194, 7c8d3be.

Verdict: ship — clean, approve.

This PR makes the built-in sinks accept the retry options buffer-size, send-timeout, and send-retry-wait. The factory now parses them in a fixed order, removes them from a copy of the options map, and checks their ranges. It also stops the send-retry-wait wait when send-timeout expires. The riskiest piece was the new early return in Retry.Send; tracing it showed no regression and no lost metrics. I found no defects.

Assumptions & unknowns

• Reviewed head 7c8d3bebf7876f0105d51d60a031bb39942bbd93 against a freshly fetched origin/main. The PR is 4 commits ahead and 0 behind.
• I couldn't run local validation because this review session's permissions don't allow go commands. All conclusions below come from reading the source, plus CI.

The one thing that could have broken, verified

I checked the new interruptible wait in Retry.Send (retry.go#L214-L222) by reading the source:

• On ctx2.Done() it returns nil. That's the same as the existing check at the top of the loop (#L206-L210).
• The pending item is still on the stack, because pop removes only an item that was sent. So the next cycle retries it, and nothing is dropped or counted twice.
• The deferred calls still reset sending and cancel ctx2. No lock is held during the wait, and the timer is stopped on the early-return path.
• Base slept without checking the timeout, then sent with an already-expired context, which logged a spurious SINK_SEND_ERROR. Head skips that send.
• The production caller passes context.Background() (monitor/level_collector.go), so only send-timeout cancels ctx2.

Verified correct

• No config that loaded before now fails. At base, all four constructors reject unknown keys with invalid option, so any retry option already failed. That includes the 0 and negative values base would otherwise have accepted.
• Retry keys don't collide with any sink's own option names. The caller's map isn't changed, because maps.Clone copies it. Nil options (a sink listed without options in monitor/loader.go) still work, because cloning, reading, and deleting are all safe on a nil map.
• log and noop still return before the retry options are parsed (factory.go#L102-L107). Custom registered sinks go through their own factory and aren't affected.
• Defaults are unchanged. An unset option stays zero, Validate accepts zero, and NewRetry replaces zero with the default. Users can no longer set a zero value explicitly. NewRetry still doesn't validate its arguments, so existing Go callers keep working.
• datadog is still built as NewDelta(NewRetry(...)), and both HTTPClient.MakeForSink and the sink constructors receive the stripped options.
• Tests:
  • TestFactoryMake_RetryOptions enters through sink.Make for all four sinks. By reading the code, it would fail on base with invalid option.
  • TestRetry_SendTimeoutBoundsRetryWait would fail with the old unconditional sleep. I couldn't confirm either by running them.
• Docs match the code: the valid ranges, defaults, the statement that a monitor sends to its sinks one at a time, and the removal of the sinks.retry section.
• CI: build, DCO, Semgrep, and zizmor pass.
• Not applicable: security, migrations, wire compatibility.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run: this review session's permissions don't allow go commands, so I relied on CI and reading the source.