✨ 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.
There was a problem hiding this comment.
I think we should also ensure that the log output is lowercased, which I am not sure happens right now.
With textlogger it can be done by making the catStat implement slog.LogValuer for example, which would also solve the problem of private attributes - ie. we would be free to make them private.
Alternatively, adding tags with lowercase keys to the struct would also work.
There was a problem hiding this comment.
In the log line, the structured log keys are lowercase. However, the catStat value string contains field names that match the casing of the struct fields (hence they are currently capitalized).
I don't personally think we need to downcase the first letter of those field names, but I'm also not really against it. I'll generally agree that the logs would look more consistent if we did.
I'll take a look at what it would take to do that. It might just be json tags.
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?
There was a problem hiding this comment.
Before, if len(packageFBC.Bundles) was 0, we would evaluate/run the predicates against a 0-length slice, which would return a 0-length slice, and then we'd return nil in line 103 (old) / line 124 (new)
So I don't think there's a behavior change here other than skipping some unnecessary predicate setup.
| @@ -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?
There was a problem hiding this comment.
We could. I felt like those other returns include context in the error message that would make it possible to figure out what happened, so that's why I only included it here.
Signed-off-by: Joe Lanford <[email protected]>
joelanford
force-pushed
the
better-logging
branch
from
382d2e6 to
67e6a22
Compare
|
New changes are detected. LGTM label has been removed. |
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