diff --git a/docs/content/about/release-notes.md b/docs/content/about/release-notes.md index 0a7a56ef..9b9b89d1 100644 --- a/docs/content/about/release-notes.md +++ b/docs/content/about/release-notes.md @@ -57,6 +57,10 @@ Default sink HTTP clients now have a 10-second whole-request timeout plus bounde * Fixed the `wait.io.table` collector after a failed read. With `truncate-table=no`, or on a read error not caused by the CMR, it returned partial metrics and no error; it now returns the error and no metrics. With `truncate-table=yes` (the default), it doesn't truncate the table after a failed read, and the truncate error policy's metric setting (`drop` or `zero`) now applies to the next sample, which covers more than one interval. Previously, a failed query or row scan was followed by one sample reported as a one-interval delta, about twice too high. A failed read is also no longer subject to the truncate error policy's `ignore` and `stop` settings: previously, a CMR timeout mid-read also failed the truncate, so `ignore` hid the error and `stop` stopped the collector. * Fixed the `repl.lag` collector's heartbeat reader, which kept running after its monitor stopped when `repl.lag` was collected at more than one level. It now stops when its monitor stops. * Changed the engine to call a collector's `Prepare` cleanup func when `Prepare` returns an error with a non-nil cleanup func, and to call the cleanup funcs of the collectors already prepared when a later collector in the plan fails. Previously they were not called, so resources started in `Prepare` leaked. +* Fixed built-in sinks rejecting the retry options `buffer-size`, `send-timeout`, and `send-retry-wait`. Set these options in each built-in sink's options. The docs previously showed a separate `sinks.retry` section, which fails with "sink retry not registered". +* Added validation of the retry options: `buffer-size` must be from 1 to 10000, `send-timeout` must be greater than zero and at most 1m, and `send-retry-wait` must be greater than zero. Other values prevent the monitor from loading. Previously a built-in sink rejected any retry option, so no config that loaded before is affected. +* Changed the retry sink to stop waiting `send-retry-wait` between sends when `send-timeout` expires. Previously a send cycle could run past `send-timeout` by up to one `send-retry-wait`. +* Added `sink.RetryArgs.Validate` and the `sink.MAX_RETRY_BUFFER_SIZE` and `sink.MAX_RETRY_SEND_TIMEOUT` constants. `sink.NewRetry` doesn't call `Validate`, so custom sinks that build `RetryArgs` from user input should call it first. ### v2.1.0 (10 Aug 2026) diff --git a/docs/content/config/quick-ref.md b/docs/content/config/quick-ref.md index 73e35182..928dec65 100644 --- a/docs/content/config/quick-ref.md +++ b/docs/content/config/quick-ref.md @@ -103,12 +103,10 @@ sinks: # No options noop: # No options - retry: - buffer-size: 60 - send-timeout: 5s - send-retry-wait: 200ms signalfx: # See Sinks > signalfx + # Built-in sinks except log and noop also accept + # retry options. See Sinks > retry tags: env: ${ENVIRONMENT:-dev} diff --git a/docs/content/sinks/chronosphere.md b/docs/content/sinks/chronosphere.md index 9f54b8a4..1edd8d9f 100644 --- a/docs/content/sinks/chronosphere.md +++ b/docs/content/sinks/chronosphere.md @@ -19,6 +19,8 @@ If you want a Chronosphere collector to scrape metrics, use [Prometheus emulatio This sink reports Prometheus-style metric names: `mysql_status_threads_running` instead of `status.global.threads_running`. It reports all [tags]({{< ref "/config/config-file#tags" >}}) as Prometheus labels. +This sink also accepts the [retry]({{< ref "retry" >}}) options `buffer-size`, `send-timeout`, and `send-retry-wait`. + ## Quick Reference ```yaml diff --git a/docs/content/sinks/datadog.md b/docs/content/sinks/datadog.md index ca1dea62..0832e3b3 100644 --- a/docs/content/sinks/datadog.md +++ b/docs/content/sinks/datadog.md @@ -28,6 +28,8 @@ Starting from v1.1, this sink now sends delta values to Datadog for counter metr If you wish to track the difference in counter values since v1.1, you can tag metrics with the blip version. +This sink also accepts the [retry]({{< ref "retry" >}}) options `buffer-size`, `send-timeout`, and `send-retry-wait`. + ## Quick Reference ```yaml diff --git a/docs/content/sinks/prom-pushgateway.md b/docs/content/sinks/prom-pushgateway.md index f822ea2d..4413b6cb 100644 --- a/docs/content/sinks/prom-pushgateway.md +++ b/docs/content/sinks/prom-pushgateway.md @@ -13,6 +13,8 @@ It uses Prometheus domain translation, so it only works with [compatible domains Currently, this sink is intended to work with [Vector by Datadog](https://vector.dev/). Therefore, it uses only `POST` and the text (exposition) protocol; it does not use RPC with protos. +This sink also accepts the [retry]({{< ref "retry" >}}) options `buffer-size`, `send-timeout`, and `send-retry-wait`. + ## Quick Reference ```yaml diff --git a/docs/content/sinks/retry.md b/docs/content/sinks/retry.md index c9dcf693..019601fd 100644 --- a/docs/content/sinks/retry.md +++ b/docs/content/sinks/retry.md @@ -3,17 +3,56 @@ title: retry --- The retry sink is a pseudo-sink that provides buffering, serialization, and retry for real sinks. -The built-in sinks, except [`log`]({{< ref "log" >}}), use a retry sink to handle those three complexities. +The built-in sinks, except [`log`]({{< ref "log" >}}) and `noop`, use a retry sink to handle those three complexities. The retry sink uses a LIFO queue (a stack) to prioritize sending the latest metrics. During a long outage of the real sink, the retry sink drops the oldest metrics and keeps the latest metrics, up to its buffer size, which is configurable. +Retry is not configured as a separate sink: a `sinks.retry` section fails with "sink retry not registered". +Set the retry options in the options of each built-in sink that uses it, alongside that sink's own options. +An invalid value prevents the monitor from loading. + ## Quick Reference ```yaml sinks: - retry: + datadog: + # datadog options buffer-size: 60 send-timeout: 5s send-retry-wait: 200ms ``` + +## Options + +### `buffer-size` + +| | | +|-|-| +|**Valid values**|Integer from 1 to 10000| +|**Default value**|60| + +Maximum number of metrics batches buffered while the real sink is failing. +When the buffer is full, the oldest batch is dropped. + +### `send-timeout` + +| | | +|-|-| +|**Valid values**|[Go duration string](https://pkg.go.dev/time#ParseDuration) greater than zero and at most 1m| +|**Default value**|5s| + +Maximum time for each send cycle, in which buffered metrics batches are sent to the real sink, newest first. +When the timeout expires, the send in progress is cancelled and the cycle ends; a real sink that ignores cancellation can run past it. +Batches not sent in time stay in the buffer for the next cycle. +A monitor finishes each send cycle before it sends metrics to its next sink, so a long timeout delays the monitor's other sinks during an outage. + +### `send-retry-wait` + +| | | +|-|-| +|**Valid values**|[Go duration string](https://pkg.go.dev/time#ParseDuration) greater than zero| +|**Default value**|200ms| + +Wait time between sends to the real sink within one send cycle. +The wait ends early when `send-timeout` expires, which ends the cycle. diff --git a/docs/content/sinks/signalfx.md b/docs/content/sinks/signalfx.md index b050b1b9..e36cba33 100644 --- a/docs/content/sinks/signalfx.md +++ b/docs/content/sinks/signalfx.md @@ -10,6 +10,8 @@ The signalfx sink sends metrics to [Splunk](https://www.splunk.com/), which [acq It reports all [tags]({{< ref "/config/config-file#tags" >}}) as dimensions. +This sink also accepts the [retry]({{< ref "retry" >}}) options `buffer-size`, `send-timeout`, and `send-retry-wait`. + ## Quick Reference ```yaml diff --git a/sink/factory.go b/sink/factory.go index 5ad4135e..937a1e95 100644 --- a/sink/factory.go +++ b/sink/factory.go @@ -5,6 +5,7 @@ package sink import ( "context" "fmt" + "maps" "strconv" "sync" "time" @@ -109,60 +110,62 @@ func (f *factory) Make(args blip.SinkFactoryArgs) (blip.Sink, error) { // Built-in sinks use Retry to serialize access and retry on Send error. // First build the specific sink, then return it wrapped in Retry. - // Parse retry options + // Parse retry options. Retry options are consumed here, so delete them + // from the options passed to the specific sink, which rejects unknown options. retryArgs := RetryArgs{ MonitorId: args.MonitorId, } - if v, ok := args.Options["buffer-size"]; ok { - n, err := strconv.Atoi(v) - if err != nil { - return nil, err + // Parse in a fixed order, not map order, so the same invalid options always + // return the same error. + sinkOpts := maps.Clone(args.Options) + for _, k := range []string{"buffer-size", "send-timeout", "send-retry-wait"} { + v, ok := sinkOpts[k] + if !ok { + continue } - if n < 0 { - return nil, fmt.Errorf("invalid retry buffer-size: %d: must be greater than zero", n) + var err error + switch k { + case "buffer-size": + retryArgs.BufferSize, err = parseRetryBufferSize(v) + case "send-timeout": + retryArgs.SendTimeout, err = parseRetryDuration(v) + case "send-retry-wait": + retryArgs.SendRetryWait, err = parseRetryDuration(v) } - retryArgs.BufferSize = uint(n) - } - if v, ok := args.Options["send-timeout"]; ok { - d, err := time.ParseDuration(v) if err != nil { - return nil, err + return nil, fmt.Errorf("invalid %s sink option %s: %w", args.SinkName, k, err) } - retryArgs.SendTimeout = d + delete(sinkOpts, k) } - if v, ok := args.Options["send-retry-wait"]; ok { - d, err := time.ParseDuration(v) - if err != nil { - return nil, err - } - retryArgs.SendRetryWait = d + if err := retryArgs.Validate(); err != nil { + return nil, fmt.Errorf("invalid %s sink option %w", args.SinkName, err) } // Make specific built-in sink var err error switch args.SinkName { case "chronosphere": - retryArgs.Sink, err = NewChronosphere(args.MonitorId, args.Options, args.Tags) + retryArgs.Sink, err = NewChronosphere(args.MonitorId, sinkOpts, args.Tags) case "signalfx": - httpClient, err := f.HTTPClient.MakeForSink("signalfx", args.MonitorId, args.Options, args.Tags) + httpClient, err := f.HTTPClient.MakeForSink("signalfx", args.MonitorId, sinkOpts, args.Tags) if err != nil { return nil, err } - retryArgs.Sink, err = NewSignalFx(args.MonitorId, args.Options, args.Tags, httpClient) + retryArgs.Sink, err = NewSignalFx(args.MonitorId, sinkOpts, args.Tags, httpClient) if err != nil { return nil, err } case "datadog": - httpClient, err := f.HTTPClient.MakeForSink("datadog", args.MonitorId, args.Options, args.Tags) + httpClient, err := f.HTTPClient.MakeForSink("datadog", args.MonitorId, sinkOpts, args.Tags) if err != nil { return nil, err } - retryArgs.Sink, err = NewDatadog(args.MonitorId, args.Options, args.Tags, httpClient) + retryArgs.Sink, err = NewDatadog(args.MonitorId, sinkOpts, args.Tags, httpClient) if err != nil { return nil, err } case "prom-pushgateway": - retryArgs.Sink, err = NewPromPushgateway(args.MonitorId, args.Options, args.Tags) + retryArgs.Sink, err = NewPromPushgateway(args.MonitorId, sinkOpts, args.Tags) default: return nil, fmt.Errorf("sink %s not registered", args.SinkName) } @@ -180,3 +183,29 @@ func (f *factory) Make(args blip.SinkFactoryArgs) (blip.Sink, error) { return NewRetry(retryArgs), nil } } + +// parseRetryBufferSize parses a buffer-size option, which must be greater than +// zero. RetryArgs.Validate checks the maximum. +func parseRetryBufferSize(v string) (uint, error) { + n, err := strconv.Atoi(v) + if err != nil { + return 0, err + } + if n <= 0 { + return 0, fmt.Errorf("%d: must be greater than zero", n) + } + return uint(n), nil +} + +// parseRetryDuration parses a send-timeout or send-retry-wait option, which must +// be greater than zero. RetryArgs.Validate checks the maximum. +func parseRetryDuration(v string) (time.Duration, error) { + d, err := time.ParseDuration(v) + if err != nil { + return 0, err + } + if d <= 0 { + return 0, fmt.Errorf("%s: must be greater than zero", v) + } + return d, nil +} diff --git a/sink/factory_test.go b/sink/factory_test.go new file mode 100644 index 00000000..5fa0b057 --- /dev/null +++ b/sink/factory_test.go @@ -0,0 +1,166 @@ +// Copyright 2026 Block, Inc. + +package sink + +import ( + "net/http" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/cashapp/blip/v2" + "github.com/cashapp/blip/v2/status" +) + +type testHTTPClientFactory struct { + opts map[string]map[string]string // sink name => options +} + +func (f *testHTTPClientFactory) MakeForSink(sinkName, monitorId string, opts, tags map[string]string) (*http.Client, error) { + f.opts[sinkName] = opts + return &http.Client{}, nil +} + +func TestFactoryMake_RetryOptions(t *testing.T) { + httpFactory := &testHTTPClientFactory{opts: map[string]map[string]string{}} + defer func(prev blip.HTTPClientFactory) { f.HTTPClient = prev }(f.HTTPClient) + f.HTTPClient = httpFactory + + retryOpts := map[string]string{ + "buffer-size": "3", + "send-timeout": "7s", + "send-retry-wait": "300ms", + } + + tests := []struct { + sinkName string + opts map[string]string // sink-specific options + http bool // uses HTTPClient.MakeForSink + }{ + {sinkName: "chronosphere", opts: map[string]string{"url": "http://127.0.0.1:3030"}}, + {sinkName: "signalfx", opts: map[string]string{"auth-token": "token"}, http: true}, + {sinkName: "datadog", opts: map[string]string{"api-key-auth": "key", "app-key-auth": "key"}, http: true}, + {sinkName: "prom-pushgateway", opts: map[string]string{"addr": "http://127.0.0.1:9091"}}, + } + for _, tt := range tests { + t.Run(tt.sinkName, func(t *testing.T) { + opts := map[string]string{} + for k, v := range tt.opts { + opts[k] = v + } + for k, v := range retryOpts { + opts[k] = v + } + + monitorId := "factory-retry-" + tt.sinkName + defer status.RemoveMonitor(monitorId) + + s, err := Make(blip.SinkFactoryArgs{ + SinkName: tt.sinkName, + MonitorId: monitorId, + Options: opts, + }) + require.NoError(t, err) + + // Monitor registers the retry buffer counter through the returned sink + cs, ok := s.(CounterSink) + require.True(t, ok, "sink is %T, expected CounterSink", s) + cs.RegisterCounters() + counter := status.SINK_RETRY_DROPPED + ":" + tt.sinkName + _, ok = status.ReportCounters(monitorId)[monitorId][counter] + assert.True(t, ok, "%s not reported", counter) + + if d, ok := s.(*Delta); ok { + s = d.sink + } + retry, ok := s.(*Retry) + require.True(t, ok, "sink is %T, expected *Retry", s) + assert.Equal(t, 3, retry.max+1) + assert.Equal(t, 3, len(retry.stack)) + assert.Equal(t, 7*time.Second, retry.sendTimeout) + assert.Equal(t, 300*time.Millisecond, retry.retryWait) + assert.Equal(t, tt.sinkName, retry.Name()) + + // Caller's options are not modified + assert.Len(t, opts, len(tt.opts)+len(retryOpts)) + + if tt.http { + assert.Equal(t, tt.opts, httpFactory.opts[tt.sinkName]) + } + }) + } +} + +func TestFactoryMake_RetryDefaults(t *testing.T) { + s, err := Make(blip.SinkFactoryArgs{ + SinkName: "chronosphere", + MonitorId: "m1", + }) + require.NoError(t, err) + + retry, ok := s.(*Retry) + require.True(t, ok, "sink is %T, expected *Retry", s) + assert.Equal(t, DEFAULT_RETRY_BUFFER_SIZE, retry.max+1) + assert.Equal(t, 5*time.Second, retry.sendTimeout) + assert.Equal(t, 200*time.Millisecond, retry.retryWait) +} + +func TestFactoryMake_InvalidRetryOptions(t *testing.T) { + tests := []struct { + option string + value string + }{ + {"buffer-size", "ten"}, + {"buffer-size", "-1"}, + {"buffer-size", "0"}, + {"buffer-size", strconv.Itoa(MAX_RETRY_BUFFER_SIZE + 1)}, + {"send-timeout", "5"}, + {"send-timeout", "-1s"}, + {"send-timeout", "0s"}, + {"send-timeout", (MAX_RETRY_SEND_TIMEOUT + time.Second).String()}, + {"send-retry-wait", "fast"}, + {"send-retry-wait", "-200ms"}, + {"send-retry-wait", "0"}, + } + for _, tt := range tests { + t.Run(tt.option+"="+tt.value, func(t *testing.T) { + _, err := Make(blip.SinkFactoryArgs{ + SinkName: "chronosphere", + MonitorId: "m1", + Options: map[string]string{tt.option: tt.value}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid chronosphere sink option "+tt.option+": ") + }) + } + + // With more than one invalid option, the error is always for the first + // option in parse order: buffer-size, send-timeout, send-retry-wait + for i := 0; i < 20; i++ { + _, err := Make(blip.SinkFactoryArgs{ + SinkName: "chronosphere", + MonitorId: "m1", + Options: map[string]string{ + "send-retry-wait": "fast", + "send-timeout": "5", + "buffer-size": "ten", + }, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid chronosphere sink option buffer-size: ") + } + + // Maximum values are valid + _, err := Make(blip.SinkFactoryArgs{ + SinkName: "chronosphere", + MonitorId: "m1", + Options: map[string]string{ + "buffer-size": strconv.Itoa(MAX_RETRY_BUFFER_SIZE), + "send-timeout": MAX_RETRY_SEND_TIMEOUT.String(), + }, + }) + require.NoError(t, err) +} diff --git a/sink/retry.go b/sink/retry.go index 31dccdc1..4f3f64c6 100644 --- a/sink/retry.go +++ b/sink/retry.go @@ -4,6 +4,7 @@ package sink import ( "context" + "fmt" "sync" "sync/atomic" "time" @@ -17,11 +18,16 @@ const ( DEFAULT_RETRY_BUFFER_SIZE = 60 DEFAULT_RETRY_SEND_TIMEOUT = "5s" DEFAULT_RETRY_SEND_RETRY_WAIT = "200ms" + + // Maximum RetryArgs values. MAX_RETRY_SEND_TIMEOUT bounds how long Send + // can block its caller. + MAX_RETRY_BUFFER_SIZE = 10000 + MAX_RETRY_SEND_TIMEOUT = time.Minute ) // Retry is a pseudo-sink that provides buffering, serialization, and retry for -// a real sink. The built-in sinks, except "log", use Retry to handle those three -// complexities. +// a real sink. The built-in sinks, except "log" and "noop", use Retry to handle +// those three complexities. // // Retry uses a LIFO queue (a stack) to prioritize sending the latest metrics. // This means that, during a long outage of the real sink, Retry drops the oldest @@ -76,6 +82,9 @@ type retryItem struct { checkpoint any } +// RetryArgs are the arguments for NewRetry. NewRetry doesn't check the ranges +// of the optional values, so callers that build RetryArgs from user input, like +// sink options, should call Validate first. type RetryArgs struct { MonitorId string // required Sink blip.Sink // required @@ -84,6 +93,27 @@ type RetryArgs struct { SendRetryWait time.Duration // optional; DEFAULT_RETRY_SEND_RETRY_WAIT } +// Validate returns an error if an optional value is out of range. Zero values +// are valid and mean the default. Errors are prefixed with the sink option +// name of the invalid value. +func (a RetryArgs) Validate() error { + if a.BufferSize > MAX_RETRY_BUFFER_SIZE { + return fmt.Errorf("buffer-size: %d: must be at most %d", a.BufferSize, MAX_RETRY_BUFFER_SIZE) + } + if a.SendTimeout < 0 { + return fmt.Errorf("send-timeout: %s: must not be negative", a.SendTimeout) + } + if a.SendTimeout > MAX_RETRY_SEND_TIMEOUT { + return fmt.Errorf("send-timeout: %s: must be at most %s", a.SendTimeout, MAX_RETRY_SEND_TIMEOUT) + } + if a.SendRetryWait < 0 { + return fmt.Errorf("send-retry-wait: %s: must not be negative", a.SendRetryWait) + } + return nil +} + +// NewRetry returns a Retry sink. It panics if a required arg is missing or +// invalid. It doesn't check optional values; see RetryArgs.Validate. func NewRetry(args RetryArgs) *Retry { // Panic if caller doesn't provide required args if args.MonitorId == "" { @@ -179,9 +209,16 @@ func (rb *Retry) Send(ctx context.Context, m *blip.Metrics) error { default: } - // Throttle between send, except on first send + // Throttle between send, except on first send. Stop waiting when + // either context is cancelled, so Send doesn't overrun sendTimeout. if n > 0 { - time.Sleep(rb.retryWait) + t := time.NewTimer(rb.retryWait) + select { + case <-ctx2.Done(): + t.Stop() + return nil + case <-t.C: + } } n += 1 diff --git a/sink/retry_test.go b/sink/retry_test.go index f2edb8a7..5a933e75 100644 --- a/sink/retry_test.go +++ b/sink/retry_test.go @@ -156,6 +156,61 @@ func TestRetry_SendTimeoutBoundsInFlightSend(t *testing.T) { } } +func TestRetry_SendTimeoutBoundsRetryWait(t *testing.T) { + // Real sink always fails, so Send waits send-retry-wait before retrying + failingSink := mock.Sink{ + SendFunc: func(ctx context.Context, m *blip.Metrics) error { + return fmt.Errorf("send failed") + }, + } + rb := NewRetry(RetryArgs{ + MonitorId: "m1", + Sink: failingSink, + BufferSize: 3, + SendTimeout: 200 * time.Millisecond, + SendRetryWait: time.Minute, + }) + + done := make(chan struct{}) + go func() { + rb.Send(context.Background(), &blip.Metrics{Level: "1"}) + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Retry.Send stalled: SendTimeout did not bound the retry wait") + } + assert.Equal(t, []string{"1", "", ""}, stack(rb)) +} + +func TestRetryArgsValidate(t *testing.T) { + sink := mock.Sink{} + valid := []RetryArgs{ + {}, // defaults + {BufferSize: MAX_RETRY_BUFFER_SIZE, SendTimeout: MAX_RETRY_SEND_TIMEOUT, SendRetryWait: time.Hour}, + } + for _, args := range valid { + assert.NoError(t, args.Validate(), "%+v", args) + } + + invalid := []RetryArgs{ + {BufferSize: MAX_RETRY_BUFFER_SIZE + 1}, + {SendTimeout: -time.Second}, + {SendTimeout: MAX_RETRY_SEND_TIMEOUT + time.Second}, + {SendRetryWait: -time.Second}, + } + for _, args := range invalid { + assert.Error(t, args.Validate(), "%+v", args) + + // NewRetry doesn't validate, so existing callers keep working + args.MonitorId = "m1" + args.Sink = sink + assert.NotPanics(t, func() { NewRetry(args) }, "%+v", args) + } +} + func TestRetryPopMiddle(t *testing.T) { // Test that popping values from the middle of the stack works. This happens // when, in this test for example, while sending metrics1, two more metrics