graphite: refactor error handling

[ifireice]

Removed Logger interface and make error handling more flexible.

For example, we could use our own logger implementation and have more control over the execution flow

package main

import (
	"context"
	"time"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/client_golang/prometheus/graphite"
	"go.uber.org/zap"
)

func main() {
	logger, _ := zap.NewProduction()
	defer logger.Sync()

	ctx := context.Background()
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	c := &Config{
			URL:              "graphite.example.org:3099",
			Gatherer:         prometheus.DefaultGatherer,
			Prefix:           "prefix",
			Interval:          5 * time.Second,
			Timeout:           2 * time.Second,
			ErrorHandling:     testCase.errorHandling,
			ErrorCallbackFunc: func(err error) {  if err != nil { logger.Error("run", zap.Error(err)); cancel() } },
		}


	b, err := graphite.NewBridge(c)
	if err != nil {
			t.Fatal(err)
		}

	b.Run(ctx)
}

[bwplotka]

I think this is fair improvement, but can we break compatibility there? I don't think so ): cc @beorn7 ?

[beorn7]

Yeah, this is a breaking change that can only happen in v2. (And even then, all users of "the old way" might not be happy that they have to change things.)

Could you add the more flexible handling in a backwards compatible way?

[ifireice]

Made error interception native to existing implementation

[bwplotka]

Amazing! It looks now much better, suggested one thing, let me know what you think 🤗

[bwplotka]

If Callback is another option after Abort or Continue, it has to control abort/continue in callback response for full flexibility:

Also let's call it "ErrorCallbackFunc" to be precise?

Suggested change

	type CallbackFunc func(error)
	type ErrorCallbackFunc func(error) (abort bool)

Alternative is to perhaps remove Callback Handler enum option and just rely on callback function to be filled AND respect continue/abort enum. The former idea gives more flexiblity though. WDYT?

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[bwplotka]

Thanks! Sorry for lag and sounds like some unwanted changes on exemplars got in? Do you mind removing them, checking my comments and rebasing? I think it's good to go otherwise!

[bwplotka]

Hm, we can't have another package main here 🤔

[bwplotka]

👍🏽

[bwplotka]

I think it would be safe to leave that panic in, especially if we would add validation to NewBridge for it, WDYT?