Skip to content

[service]: use configured logger whenever possible #13081

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 27, 2025
Merged

[service]: use configured logger whenever possible #13081

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
caffdbc
[service]: use configured logger whenever possible
TylerHelmuth May 22, 2025
f8453ca
changelog
TylerHelmuth May 22, 2025
ba337f1
use defer function
TylerHelmuth May 27, 2025
b0a9b66
Merge branch 'main' into tyler.service-log-errors
TylerHelmuth May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/tyler.service-log-errors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: service

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Use configured loggers to log errors as soon as it is available

# One or more tracking issues or pull requests related to the change
issues: [13081]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
6 changes: 6 additions & 0 deletions service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@

tracerProvider, err := telFactory.CreateTracerProvider(ctx, telset, &cfg.Telemetry)
if err != nil {
logger.Error("failed to create tracer provider", zap.Error(err))

Check warning on line 195 in service/service.go

View check run for this annotation

Codecov / codecov/patch

service/service.go#L195 

Added line #L195 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

err = multierr.Append(err, sdk.Shutdown(ctx))
return nil, fmt.Errorf("failed to create tracer provider: %w", err)
}
Expand All @@ -200,6 +201,7 @@

mp, err := telFactory.CreateMeterProvider(ctx, telset, &cfg.Telemetry)
if err != nil {
logger.Error("failed to create meter provider", zap.Error(err))

Check warning on line 204 in service/service.go

View check run for this annotation

Codecov / codecov/patch

service/service.go#L204 

Added line #L204 was not covered by tests
err = multierr.Append(err, sdk.Shutdown(ctx))
return nil, fmt.Errorf("failed to create meter provider: %w", err)
}
Expand All @@ -218,18 +220,22 @@
})

if err = srv.initGraph(ctx, cfg); err != nil {
logger.Error("failed to initialize service graph", zap.Error(err))
err = multierr.Append(err, srv.shutdownTelemetry(ctx))
return nil, err
}

// process the configuration and initialize the pipeline
if err = srv.initExtensions(ctx, cfg.Extensions); err != nil {
logger.Error("failed to initialize extensions", zap.Error(err))

Check warning on line 230 in service/service.go

View check run for this annotation

Codecov / codecov/patch

service/service.go#L230 

Added line #L230 was not covered by tests
err = multierr.Append(err, srv.shutdownTelemetry(ctx))
return nil, err
}

if cfg.Telemetry.Metrics.Level != configtelemetry.LevelNone && (len(mpConfig.Readers) != 0 || cfg.Telemetry.Metrics.Address != "") {
if err = proctelemetry.RegisterProcessMetrics(srv.telemetrySettings); err != nil {
logger.Error("failed to register process metrics", zap.Error(err))
err = multierr.Append(err, srv.shutdownTelemetry(ctx))

Check warning on line 238 in service/service.go

View check run for this annotation

Codecov / codecov/patch

service/service.go#L237-L238 

Added lines #L237 - L238 were not covered by tests
return nil, fmt.Errorf("failed to register process metrics: %w", err)
}
}
Expand Down
Loading