Skip to content
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

Jump to bottom

[otelcol] Allow confmap to write logs using configured logger #10008

Closed
wants to merge 7 commits into from
Closed

[otelcol] Allow confmap to write logs using configured logger #10008

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
562baaf
Write logs once we have a logger
TylerHelmuth Apr 19, 2024
31b4ef9
Add a test
TylerHelmuth Apr 23, 2024
d23b50a
Use `Greater` assertion
TylerHelmuth Apr 23, 2024
47b5f0c
Update test to use Provider that logs expected message
TylerHelmuth Apr 25, 2024
8f3211e
Improve test
TylerHelmuth Apr 26, 2024
8f25cb3
Merge branch 'main' into pass-logger-to-providersettings-save-logs
TylerHelmuth Apr 26, 2024
8e9bce4
Update otelcol/collector_test.go
TylerHelmuth Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion otelcol/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

"go.uber.org/multierr"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"
Copy link
Contributor

Choose a reason for hiding this comment

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

bringing in a test import here seems a bit strange. from the observer docs:

It's useful for applications that want to unit test their log output without tying their tests to a particular output encoding.

I worry that this package may change in the future causing issue, but maybe it's not a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're certainly using the package in a use case I think they didn't intend. I still think the simplicity of #10007 is most appropriate as it is the collector making logging decisions before being told how to log. It keeps the logs in proper order as well

Copy link
Member

Choose a reason for hiding this comment

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

I worry that this package may change in the future causing issue, but maybe it's not a problem?

I don't think that should be a problem since (i) the fact that we use this package is an implementation detail and (ii) even if the package disappeared we could rebuild it ourselves (it does not depend on any internal bits of zap)


"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/confmap"
Expand Down Expand Up @@ -109,14 +110,17 @@ type Collector struct {
signalsChannel chan os.Signal
// asyncErrorChannel is used to signal a fatal error from any component.
asyncErrorChannel chan error

ol *observer.ObservedLogs
}

// NewCollector creates and returns a new instance of Collector.
func NewCollector(set CollectorSettings) (*Collector, error) {
var err error
configProvider := set.ConfigProvider

set.ConfigProviderSettings.ResolverSettings.ProviderSettings = confmap.ProviderSettings{Logger: zap.NewNop()}
core, ol := observer.New(zap.DebugLevel)
set.ConfigProviderSettings.ResolverSettings.ProviderSettings = confmap.ProviderSettings{Logger: zap.New(core)}
set.ConfigProviderSettings.ResolverSettings.ConverterSettings = confmap.ConverterSettings{}
mx-psi marked this conversation as resolved.
Show resolved Hide resolved

if configProvider == nil {
Expand All @@ -137,6 +141,7 @@ func NewCollector(set CollectorSettings) (*Collector, error) {
signalsChannel: make(chan os.Signal, 3),
asyncErrorChannel: make(chan error),
configProvider: configProvider,
ol: ol,
}, nil
}

Expand Down Expand Up @@ -202,6 +207,12 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error {
return err
}

if col.ol != nil {
for _, log := range col.ol.All() {
col.service.Logger().Log(log.Level, log.Message)
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should use the core directly here via its Check and Write methods. That way we would preserve the exact logged message, including the timestamp and stack trace. I think it can be done, but it's not trivial since there is some logic we would have to copy from here.

Do you think it's worth the effort?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would having out-of-order timestamps be a problem? I dont think stacktraces is a valid scenario bc currently if the confmap errors its logs will not get written.

Copy link
Member

Choose a reason for hiding this comment

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

I dont think stacktraces is a valid scenario bc currently if the confmap errors its logs will not get written.

I don't get what you mean by this. What I meant is that instead of having otelcol@v0.98.0/collector:XX we would have the actual line where this log came from.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at #10056 this is actually necessary since we are not logging the fields (if you compare the screenshots, with this PR you don't know what env var is unset)

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 really don't like the idea of duplicating functionality that exists in zapcore already

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I can fix the field issue by passing in log.Context... as the last param.

Copy link
Member

Choose a reason for hiding this comment

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

I really don't like the idea of duplicating functionality that exists in zapcore already

To be clear: it does not exist in zapcore, it exists in *zap.Logger

}
}

if !col.set.SkipSettingGRPCLogger {
grpclog.SetLogger(col.service.Logger(), cfg.Service.Telemetry.Logs.Level)
}
Expand Down