[service]: use configured logger whenever possible #13081
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13081 +/- ##
==========================================
+ Coverage 91.58% 91.59% +0.01%
==========================================
Files 505 505
Lines 28476 28479 +3
==========================================
+ Hits 26079 26085 +6
+ Misses 1883 1880 -3
Partials 514 514 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
songy23
added
the
collector-telemetry
|
Another approach to this, if we don't like logging an error we're returning, is to refactor so that the logger and all the sdk are instantiated outside the service and passed into the service via settings. Then that logger can be used for the error returned by New instead of the fallback logger. |
service/service.go
Outdated
| @@ -192,6 +192,7 @@ func New(ctx context.Context, set Settings, cfg Config) (*Service, error) { | |||
|
|
|||
| tracerProvider, err := telFactory.CreateTracerProvider(ctx, telset, &cfg.Telemetry) | |||
| if err != nil { | |||
| logger.Error("failed to create tracer provider", zap.Error(err)) | |||
There was a problem hiding this comment.
One alternative way of doing this is to do something like this after line 190:
defer func(){
if err != nil {
logger.Error("error found during service initialization", zap.Error(err))
}
}()(the message is a bit less specific but at least we don't have to add a new log on each place
There was a problem hiding this comment.
either works, one thing i would ask though is if the defer func is used, please add a comment otherwise i'll ask myself the same "why is this a defer func() here" every time i come across the code :D
There was a problem hiding this comment.
I like this idea as it means that any future errors added after the logging initialized don't have to worry about adding their own logging statement.
There was a problem hiding this comment.
@mx-psi updated. One downside to this strategy is the error returned from sdk.Shutdown cannot be returned from the function. We might be able to use named return values, but I dont care for those.
There was a problem hiding this comment.
I feel like this should be fine, maybe the only case I would be concerned about is if there are no other errors other than the Shutdown one
I would personally prefer this, see #4970 for some discussion on how to do this, though it would be a bigger refactor |
I ultimately prefer this as well, but I think logging the error ourselves for now helps a lot, especially for the opampsupervisor. I can also volunteer to start moving this issue forward again if there is capacity to review, but I think merging this now is worth it. |
Happy to review PRs related to this |
Description
I was using the supervised collector today and ran into an issue where the agent (collector) was crashing on startup and I wasn't seeing the logs exported via the configured logger, but I knew it existed bc I was getting the
Setting up own telemetry...log.Turns out in
service.New, once we've created the logger, we aren't using it to log and following errors. Instead, they are being returned byservice.Newand handled by the fallbackLogger.I propose that, since we have a logger, we use it.
As a followup it would be nice if any confmap errors could be reported using the instantiated logger, but that would be a bigger refactor.
Testing
Tested locally with the following config:
console output:
Proof that the error log exported:
