✨ improve logging: catalog http server, op-con resolver #1564
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
- Coverage 66.68% 66.52% -0.17%
==========================================
Files 57 57
Lines 4584 4645 +61
==========================================
+ Hits 3057 3090 +33
- Misses 1302 1329 +27
- Partials 225 226 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -65,6 +66,15 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1.ClusterExtensio | |||
| } | |||
| } | |||
|
|
|||
| type catStat struct { | |||
There was a problem hiding this comment.
WDYT about extracting this to package-level and making attributes private?
There was a problem hiding this comment.
I started out with private attributes, but then when these are eventually passed into l.Info(), logr only outputs the exported attributes. We could write more code to specifically log each unexported field, but that would be brittle if we ever add new fields.
Not highly opposed to extracting to package level, but I didn't see any use case where this would be used outside the function, so I figured the more local, the better.
There was a problem hiding this comment.
you're right, though it's actually the zap backend sink that controller-runtime/log uses which requires public fields - see go-logr/logr#142
Regarding the declaration, sure it can be extracted in the future if there's a need. Personally I just find it more readable with type declarations done on the outside and functions/methods kept smaller.
There was a problem hiding this comment.
Hmm, I thought I had switched everything over to the klog backend a month or so ago.
Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
better-logging
branch
from
5ed2b26 to
382d2e6
Compare
|
/lgtm |
| if isFBCEmpty(packageFBC) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This early exit did not exist before. Are we worried about a change in behavior?
| @@ -158,6 +179,7 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1.ClusterExtensio | |||
|
|
|||
| // Check for ambiguity | |||
| if len(resolvedBundles) != 1 { | |||
| l.Info("resolution failed", "stats", catStats) | |||
There was a problem hiding this comment.
Do we want this before all error returns from this point forward?
Description
This adds more logging to help administrators troubleshoot issues resolving bundles from catalogs. Specifically, it:
This should help administrators narrow down which stage of resolution should be investigated when unexpected resolution errors occur.
Reviewer Checklist