From 707a76763eb2098ad39129d346c0e91d4c032dc4 Mon Sep 17 00:00:00 2001 From: Darya Melentsova Date: Sun, 30 Apr 2023 16:40:36 +0300 Subject: [PATCH] graphite: refactor error handling Signed-off-by: ifireice Signed-off-by: Darya Melentsova --- prometheus/graphite/bridge.go | 62 ++++++++---------------------- prometheus/graphite/bridge_test.go | 46 +++++++++++++++++----- 2 files changed, 52 insertions(+), 56 deletions(-) diff --git a/prometheus/graphite/bridge.go b/prometheus/graphite/bridge.go index 84eac0d..b49314a 100644 --- a/prometheus/graphite/bridge.go +++ b/prometheus/graphite/bridge.go @@ -38,19 +38,11 @@ const ( millisecondsPerSecond = 1000 ) -// HandlerErrorHandling defines how a Handler serving metrics will handle -// errors. -type HandlerErrorHandling int +// ErrorHandler is a function that handles errors +type ErrorHandler func(err error) -// These constants cause handlers serving metrics to behave as described if -// errors are encountered. -const ( - // Ignore errors and try to push as many metrics to Graphite as possible. - ContinueOnError HandlerErrorHandling = iota - - // Abort the push to Graphite upon the first error encountered. - AbortOnError -) +// DefaultErrorHandler skips received errors +var DefaultErrorHandler = func(err error) {} // Config defines the Graphite bridge config. type Config struct { @@ -72,13 +64,8 @@ type Config struct { // The Gatherer to use for metrics. Defaults to prometheus.DefaultGatherer. Gatherer prometheus.Gatherer - // The logger that messages are written to. Defaults to no logging. - Logger Logger - - // ErrorHandling defines how errors are handled. Note that errors are - // logged regardless of the configured ErrorHandling provided Logger - // is not nil. - ErrorHandling HandlerErrorHandling + // ErrorHandler defines how errors are handled. + ErrorHandler ErrorHandler } // Bridge pushes metrics to the configured Graphite server. @@ -89,19 +76,11 @@ type Bridge struct { interval time.Duration timeout time.Duration - errorHandling HandlerErrorHandling - logger Logger + errorHandler ErrorHandler g prometheus.Gatherer } -// Logger is the minimal interface Bridge needs for logging. Note that -// log.Logger from the standard library implements this interface, and it is -// easy to implement by custom loggers, if they don't do so already anyway. -type Logger interface { - Println(v ...interface{}) -} - // NewBridge returns a pointer to a new Bridge struct. func NewBridge(c *Config) (*Bridge, error) { b := &Bridge{} @@ -119,10 +98,6 @@ func NewBridge(c *Config) (*Bridge, error) { b.g = c.Gatherer } - if c.Logger != nil { - b.logger = c.Logger - } - if c.Prefix != "" { b.prefix = c.Prefix } @@ -140,7 +115,7 @@ func NewBridge(c *Config) (*Bridge, error) { b.timeout = c.Timeout } - b.errorHandling = c.ErrorHandling + b.errorHandler = c.ErrorHandler return b, nil } @@ -153,9 +128,7 @@ func (b *Bridge) Run(ctx context.Context) { for { select { case <-ticker.C: - if err := b.Push(); err != nil && b.logger != nil { - b.logger.Println("error pushing to Graphite:", err) - } + b.errorHandler(b.Push()) case <-ctx.Done(): return } @@ -165,17 +138,12 @@ func (b *Bridge) Run(ctx context.Context) { // Push pushes Prometheus metrics to the configured Graphite server. func (b *Bridge) Push() error { mfs, err := b.g.Gather() - if err != nil || len(mfs) == 0 { - switch b.errorHandling { - case AbortOnError: - return err - case ContinueOnError: - if b.logger != nil { - b.logger.Println("continue on error:", err) - } - default: - panic("unrecognized error handling value") - } + if err != nil { + return err + } + + if len(mfs) == 0 { + return nil } conn, err := net.DialTimeout("tcp", b.url, b.timeout) diff --git a/prometheus/graphite/bridge_test.go b/prometheus/graphite/bridge_test.go index df0cfff..4dc3a20 100644 --- a/prometheus/graphite/bridge_test.go +++ b/prometheus/graphite/bridge_test.go @@ -19,9 +19,7 @@ import ( "context" "fmt" "io" - "log" "net" - "os" "reflect" "regexp" "sort" @@ -438,13 +436,12 @@ type mockGraphite struct { func ExampleBridge() { b, err := NewBridge(&Config{ - URL: "graphite.example.org:3099", - Gatherer: prometheus.DefaultGatherer, - Prefix: "prefix", - Interval: 15 * time.Second, - Timeout: 10 * time.Second, - ErrorHandling: AbortOnError, - Logger: log.New(os.Stdout, "graphite bridge: ", log.Lshortfile), + URL: "graphite.example.org:3099", + Gatherer: prometheus.DefaultGatherer, + Prefix: "prefix", + Interval: 15 * time.Second, + Timeout: 10 * time.Second, + ErrorHandler: func(err error) {}, }) if err != nil { panic(err) @@ -467,3 +464,34 @@ func ExampleBridge() { // Start pushing metrics to Graphite in the Run() loop. b.Run(ctx) } + +func TestErrorHandler(t *testing.T) { + var internalError error + c := &Config{ + URL: "graphite.example.org:3099", + Gatherer: prometheus.DefaultGatherer, + Prefix: "prefix", + Interval: 5 * time.Second, + Timeout: 2 * time.Second, + ErrorHandler: func(err error) { internalError = err }, + } + b, err := NewBridge(c) + if err != nil { + panic(err) + } + + // Create a Context to control stopping the Run() loop that pushes + // metrics to Graphite. Multiplied by 2, because we need Run to be executed at least one time. + ctx, cancel := context.WithTimeout(context.Background(), c.Interval*2) + defer cancel() + + // Start pushing metrics to Graphite in the Run() loop. + b.Run(ctx) + + // There are obviously no hosts like "graphite.example.com" available during the tests. + expError := fmt.Errorf("dial tcp: lookup graphite.example.org: no such host") + + if internalError.Error() != expError.Error() { + t.Fatalf("Expected: '%s', actual: '%s'", expError, internalError) + } +}