Clean up logging in controller and wrap errors#132
Conversation
| if cacheErr != nil && !storage.IsNotFound(cacheErr) { | ||
| logger.Warn("GetChangedTargets: Failed to read from cache, proceeding to compute", zap.Error(cacheErr)) | ||
| logger.Error("GetChangedTargets: failed to read from cache", zap.Error(cacheErr)) | ||
| return fmt.Errorf("failed to read from cache: %w", cacheErr) |
There was a problem hiding this comment.
I think we should still fall through and continue to compute in this case if storage read errors out
There was a problem hiding this comment.
Yea I think if there's an actual storage failure, it'll be caught in getGraph anyways.
Update Update Update Update Update logging Update Update Update Update Update Update
| }() | ||
| if err := validateGetChangedTargetsRequest(request); err != nil { | ||
| return common.WithReason(common.FailureReasonValidation, common.ErrorTypeUser, err) | ||
| c.logger.Error("GetChangedTargets: validation failed", zap.Error(err)) |
There was a problem hiding this comment.
please don't log and return the errors. Suppose you have a chain of funcs each returning an error:
func1 -> func2 -> func3
and you log before returning the error each time.
You'll end up with logs that look like:
"func3: some error happened"
"func2: error while doing something: func3: some error happened"
"func1: another thing: func2: error while doing something: func3: some error happened"
This is completely redundant and likely not what you want.
There was a problem hiding this comment.
All errors are logged once at the controller level now. The rest of the functions errors logs were removed #127. Shouldn't we have service logs for errors at this level?
| if err != nil { | ||
| logger.Error("GetChangedTargets: Failed to read revision treehash", zap.Error(err)) | ||
| return common.WithReason(failureReasonTreehashRead, common.ErrorTypeInfra, err) | ||
| logger.Error("GetChangedTargets: failed to read revision treehash", zap.Error(err)) |
4 participants
Uh oh!
There was an error while loading. Please reload this page.