Disable stacktrace from reconcile errors #49
Conversation
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
|
|
|
This PR changes the error logs, from: to: |
|
Another approach could be to provide the functionality from crossplane-runtime like here (which still uses old controller runtime, but can be updated to new method), but I don't see much benefit in doing so since we still need to consume it from other repos. |
|
I don't personally have any issue with seeing stack traces when I enable debug level logging, so personally I'm not convinced this really needs fixing. If we make these changes to our Crossplane logger, do they also apply to the controller-runtime logger? |
Change on Crossplane logger introduces a
While working on the issue, I see that the frequency of errors with stack traces are quite low, so honestly I am also not sure that this is really something that needs fixing. May be we should wait for @jbw976 opinion in this regard. |
|
I definitely still vote for fixing this issue as I have found it to be confusing and misleading for potential users. In the current state of logging, a full call stack gets printed out for simple transient errors that will easily be resolved on the next reconcile. When users look at the logs they are immediately drawn to the big scary call stacks which are actually a red herring and distract from other true/real problems. Beyond that, the call stacks provide no real value either. If you look at the stack, it's 90% machinery for handling reconcile callbacks and the only real information learned is that it was an error from reconciling. We don't need an entire scary and distracting call stack to tell us that 🤓 |
| lvl := zap.NewAtomicLevelAt(zap.FatalLevel) | ||
| o.StacktraceLevel = &lvl | ||
| } | ||
| zl := runtimezap.New(o) |
There was a problem hiding this comment.
i sort of like the idea of having these common options, that will get reused across all the controller based stacks we're building right now, in a common and reusable place like crossplane-runtime. Then these consumers, such as stack-gcp/stack-aws/stack-azure would just call the single common function from crossplane-runtime to get an initialized log instance with a single line.
I see you started this common effort with crossplane/crossplane-runtime#47. Perhaps we can continue pursuing that?
|
@jbw976 Are you sure debug level logging is the place we should be focusing on simplifying and making (arguably) less scary at the expense of becoming (also arguably) less informative? Perhaps I spent too much time in the Java world, but when I am debugging a thing I prefer more context, and definitely feel that stack traces are helpful. They let me easily find the code and context of the call I may be concerned about. I agree with the sentiment of making it easier for folks to understand what is going on under the covers in Crossplane, but I don't think debug level logging is the place for that. I like the way @suskin framed this problem in https://github.com/crossplaneio/crossplane/pull/858/files#r330818106. If we're optimising for case 1 (users need to figure out why stuff isn't working as expected) I suggest we invest our time in adding support for emitting informative Kubernetes events. If we're optimising for scenarios 3 or 4 (infrastructure operators or developers deep debugging Crossplane) then I suggest more context and information (i.e. traces) is better. |
|
One information that could help to reach an agreement here is, we still have stacktraces available under |
|
I'm going to close this issue in accordance with our discussion on crossplane/crossplane#529. |
Signed-off-by: Hasan Turken turkenh@gmail.com
Description of your changes
This PR disables stacktraces from reconcile errors (below Fatal) by using new functionality added to controller runtime with this PR.
This requires updating controller runtime dependency as v0.2.2.
Related to crossplane/crossplane#529
Checklist
I have:
make reviewableto ensure this PR is ready for review.app.yamlto include any new role permissions.