-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
graphite: refactor error handling #1263
base: main
Are you sure you want to change the base?
Conversation
675545c
to
252ec74
Compare
I think this is fair improvement, but can we break compatibility there? I don't think so ): cc @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? |
Made error interception native to existing implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! It looks now much better, suggested one thing, let me know what you think 🤗
prometheus/graphite/bridge.go
Outdated
@@ -102,6 +109,9 @@ type Logger interface { | |||
Println(v ...interface{}) | |||
} | |||
|
|||
// CallbackFunc is a special type for callback functions | |||
type CallbackFunc func(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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?
Signed-off-by: ifireice <ifireice@gmail.com> Signed-off-by: Darya Melentsova <ifireice@gmail.com>
Signed-off-by: Darya Melentsova <ifireice@gmail.com>
Signed-off-by: Darya Melentsova <ifireice@gmail.com>
Signed-off-by: Darya Melentsova <ifireice@gmail.com>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -14,6 +14,54 @@ | |||
// A simple example of how to record a latency metric with exemplars, using a fictional id | |||
// as a prometheus label. | |||
|
|||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we can't have another package main here 🤔
@@ -79,6 +79,9 @@ type Config struct { | |||
// logged regardless of the configured ErrorHandling provided Logger | |||
// is not nil. | |||
ErrorHandling HandlerErrorHandling | |||
|
|||
// ErrorCallbackFunc is a callback function that can be executed when error is occurred | |||
ErrorCallbackFunc ErrorCallbackFunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
return err | ||
case ContinueOnError: | ||
if b.logger != nil { | ||
b.logger.Println("continue on error:", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safe to leave that panic in, especially if we would add validation to NewBridge for it, WDYT?
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