-
Notifications
You must be signed in to change notification settings - Fork 43
feat: add logging on provider state transitions #1444
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
Conversation
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
} else { | ||
providerName = delegate.getMetadata().getName(); | ||
} | ||
log.info("Provider {} transitioned from state {} to state {}", providerName, oldState, state); |
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 this should be a debug log.
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 understand why you'd say so, and I think I would agree most times, but the point of this change was to get better visibly into state transitions, particularly for when providers become ready again. Because this isn't in a hot code path, I think it might be worthwhile to have at info
level. We've seen internally that such a log would be very useful to confirm the provider is back in a good state.
If we make it debug, we'll never see it "after the fact" if logging it set to normal level, so I think it will make troubleshooting things like connection issues harder. I think the chances of spam here are very low in normal situations.
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 am not sure, we need this log all the time. imho I think it is important when we are going from error back to ready - but the other transitions are not as important to see. Stale can happen, we have a gracePeriod, I do not need to know it. but after an error, I need to know when got back to ready
Signed-off-by: christian.lutnik <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1444 +/- ##
============================================
+ Coverage 92.91% 93.31% +0.39%
- Complexity 478 485 +7
============================================
Files 45 45
Lines 1158 1167 +9
Branches 99 101 +2
============================================
+ Hits 1076 1089 +13
+ Misses 53 48 -5
- Partials 29 30 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR
Adds logging on state transitions. This way errors and their recovery can be retraced better.
Related Issues
None