Do we really need this? Is it really more valuable than simply logging at the callsite? It's very custom and very convoluted and it just ends up with a log line in the end.

@lavalamp @brendandburns @smarterclayton

Do we really need this? Is it really more valuable than simply logging at the callsite? It's very custom and very convoluted and it just ends up with a log line in the end.

@lavalamp @brendandburns @smarterclayton

This provides an injection point for enabling things like sentry monitoring.

We actively use this hook on several production and preproduction clusters to provide failure analysis of controllers and "non obvious fault" conditions.

I have a long running todo to move the sentry hook upstream and also tee events to sentry as plugins.

@thockin either way, it's a big code smell (something is probably wrong if one's code "can't return an error"). But this is strictly better than a log message everywhere, because it's easy to find all the places that use it.

Controllers are are a classic "can't return an error" place, I think we
have very few other scenarios. They can emit an event - in which case this
method should be stubbed to emit an event. But the controller error could
be because the apiserver is down - in which case we probably want to hold
the event locally. Technically, the sentry implementation backing this is
an event recorded, it just happens to be one that is sent somewhere other
than the master, which in the past we discussed as a Tee at the event
recorder (it also handles and reports panics). I'm not sure we want to
fire events when we panic, but at the same time, there's no reason we can't.

On Wed, Jan 20, 2016 at 5:48 PM, Daniel Smith notifications@github.com
wrote:

In pkg/util/runtime/runtime.go
#19778 (comment)
:

•       break
  

•   }
  

•   callers = callers + fmt.Sprintf("%v:%v\n", file, line)
  

• }
• glog.Errorf("Recovered from panic: %#v (%v)\n%v", r, r, callers)
  +}

+// ErrorHandlers is a list of functions which will be invoked when an unreturnable
+// error occurs.
+var ErrorHandlers = []func(error){logError}
+
+// HandlerError is a method to invoke when a non-user facing piece of code cannot
+// return an error and needs to indicate it has been ignored. Invoking this method
+// is preferable to logging the error - the default behavior is to log but the
+// errors may be sent to a remote server for analysis.
+func HandleError(err error) {

@thockin https://github.com/thockin either way, it's a big code smell
(something is probably wrong if one's code "can't return an error"). But
this is strictly better than a log message everywhere, because it's easy to
find all the places that use it.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/19778/files#r50333611.

On Wed, Jan 20, 2016 at 5:18 AM, David Eads notifications@github.com wrote:

+// HandlerError is a method to invoke when a non-user facing piece of code cannot
+// return an error and needs to indicate it has been ignored. Invoking this method
+// is preferable to logging the error - the default behavior is to log but the
+// errors may be sent to a remote server for analysis.
+func HandleError(err error) {

Do we really need this? Is it really more valuable than simply logging at the callsite? It's very custom and very convoluted and it just ends up with a log line in the end.

@lavalamp @brendandburns @smarterclayton

This provides an injection point for enabling things like sentry monitoring.

I get that, but is it REALLY useful? It is certainly not consistently
applied. Heck, I didn't even know about it...

Sure, but we should maybe spread the word on when and how to use this. I
didn't know it existed, and I still don't think I know how to apply it.

On Wed, Jan 20, 2016 at 2:48 PM, Daniel Smith notifications@github.com
wrote:

In pkg/util/runtime/runtime.go
#19778 (comment)
:

•       break
  

•   }
  

•   callers = callers + fmt.Sprintf("%v:%v\n", file, line)
  

• }
• glog.Errorf("Recovered from panic: %#v (%v)\n%v", r, r, callers)
  +}

+// ErrorHandlers is a list of functions which will be invoked when an unreturnable
+// error occurs.
+var ErrorHandlers = []func(error){logError}
+
+// HandlerError is a method to invoke when a non-user facing piece of code cannot
+// return an error and needs to indicate it has been ignored. Invoking this method
+// is preferable to logging the error - the default behavior is to log but the
+// errors may be sent to a remote server for analysis.
+func HandleError(err error) {

@thockin https://github.com/thockin either way, it's a big code smell
(something is probably wrong if one's code "can't return an error"). But
this is strictly better than a log message everywhere, because it's easy to
find all the places that use it.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/19778/files#r50333611.