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.

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion otelcol/collector.go
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
56 changes: 56 additions & 0 deletions otelcol/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/confmap"
Expand Down Expand Up @@ -476,6 +477,61 @@ func TestPassConfmapToServiceFailure(t *testing.T) {
require.Error(t, err)
}

type provider struct {
logger *zap.Logger
}

func newWithSettings(ps confmap.ProviderSettings) confmap.Provider {
return &provider{
logger: ps.Logger,
}
}

func newFactory() confmap.ProviderFactory {
return confmap.NewProviderFactory(newWithSettings)
}

func (p *provider) Retrieve(_ context.Context, uri string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) {
p.logger.Info("this is a test log")
return confmap.NewRetrieved("test")
}

func (*provider) Scheme() string {
return "test"
}

func (*provider) Shutdown(context.Context) error {
return nil
}

func TestCollectorConfmapLogs(t *testing.T) {
configProviderSettings := newDefaultConfigProviderSettings([]string{filepath.Join("testdata", "otelcol-confmap-logging.yaml")})
configProviderSettings.ResolverSettings.ProviderFactories = append(configProviderSettings.ResolverSettings.ProviderFactories, newFactory())

set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: nopFactories,
ConfigProviderSettings: configProviderSettings,
}
col, err := NewCollector(set)
require.NoError(t, err)

wg := startCollector(context.Background(), t, col)

col.Shutdown()
wg.Wait()
assert.Equal(t, StateClosed, col.GetState())

require.NotNil(t, col.ol)
require.Greater(t, col.ol.Len(), 0)

messages := make([]string, 0, col.ol.Len())
for _, l := range col.ol.All() {
messages = append(messages, l.Message)
}
assert.Contains(t, messages, "this is a test log")
}

func startCollector(ctx context.Context, t *testing.T, col *Collector) *sync.WaitGroup {
wg := &sync.WaitGroup{}
wg.Add(1)
Expand Down
34 changes: 34 additions & 0 deletions otelcol/testdata/otelcol-confmap-logging.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
receivers:
nop:

processors:
nop:

exporters:
nop:

extensions:
nop:

connectors:
nop/con:

service:
telemetry:
logs:
initial_fields:
"test": ${test:ANYTHING}
extensions: [nop]
pipelines:
traces:
receivers: [nop]
processors: [nop]
exporters: [nop, nop/con]
metrics:
receivers: [nop]
processors: [nop]
exporters: [nop]
logs:
receivers: [nop, nop/con]
processors: [nop]
exporters: [nop]
Loading