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

[service] add logger provider configuration support #10544

Merged
merged 14 commits into from
Oct 23, 2024
Merged

[service] add logger provider configuration support #10544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5538d84
[service] add logger provider configuration support
codeboten Jul 4, 2024
24d1479
check if there's processors
codeboten Jul 8, 2024
57d7f9d
update name
codeboten Sep 19, 2024
67f0d1e
add changelog
codeboten Sep 19, 2024
e973561
fix lint
codeboten Sep 19, 2024
bd41890
fix
codeboten Oct 3, 2024
1f08e89
move logic to config SDK into block checking for processors
codeboten Oct 3, 2024
c10cbd5
lint
codeboten Oct 3, 2024
ca20cb4
bump to otelzap 0.6.0
codeboten Oct 21, 2024
0d370ba
update logger to use NewTee to preserve existing logger config
codeboten Oct 21, 2024
02461fc
add test, return logger provider in factory
codeboten Oct 22, 2024
ba20612
tidy
codeboten Oct 22, 2024
1eef143
fix lint
codeboten Oct 22, 2024
809edbe
Merge branch 'main' into codeboten/add-log-record-processor-config
codeboten Oct 23, 2024
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/codeboten_add-log-record-processor-config.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: add support for using the otelzap bridge and emit logs using the OTel Go SDK

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

# (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: []
1 change: 1 addition & 0 deletions cmd/otelcorecol/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ require (
go.opentelemetry.io/collector/receiver/receiverprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/semconv v0.112.0 // indirect
go.opentelemetry.io/collector/service v0.112.0 // indirect
go.opentelemetry.io/contrib/bridges/otelzap v0.6.0 // indirect
go.opentelemetry.io/contrib/config v0.10.0 // indirect
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.56.0 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.56.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions cmd/otelcorecol/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions internal/e2e/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ require (
go.opentelemetry.io/collector/processor/processortest v0.112.0 // indirect
go.opentelemetry.io/collector/receiver/receiverprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/semconv v0.112.0 // indirect
go.opentelemetry.io/contrib/bridges/otelzap v0.6.0 // indirect
go.opentelemetry.io/contrib/config v0.10.0 // indirect
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.56.0 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.56.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions internal/e2e/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions otelcol/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ require (
go.opentelemetry.io/collector/processor/processorprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/receiver/receiverprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/semconv v0.112.0 // indirect
go.opentelemetry.io/contrib/bridges/otelzap v0.6.0 // indirect
go.opentelemetry.io/contrib/propagators/b3 v1.31.0 // indirect
go.opentelemetry.io/otel v1.31.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.7.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions otelcol/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions otelcol/otelcoltest/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ require (
go.opentelemetry.io/collector/processor/processorprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/receiver/receiverprofiles v0.112.0 // indirect
go.opentelemetry.io/collector/semconv v0.112.0 // indirect
go.opentelemetry.io/contrib/bridges/otelzap v0.6.0 // indirect
go.opentelemetry.io/contrib/config v0.10.0 // indirect
go.opentelemetry.io/contrib/propagators/b3 v1.31.0 // indirect
go.opentelemetry.io/otel v1.31.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions otelcol/otelcoltest/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion service/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,15 @@ require (
go.opentelemetry.io/collector/receiver v0.112.0
go.opentelemetry.io/collector/receiver/receiverprofiles v0.112.0
go.opentelemetry.io/collector/semconv v0.112.0
go.opentelemetry.io/contrib/bridges/otelzap v0.6.0
go.opentelemetry.io/contrib/config v0.10.0
go.opentelemetry.io/contrib/propagators/b3 v1.31.0
go.opentelemetry.io/otel v1.31.0
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.31.0
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.31.0
go.opentelemetry.io/otel/exporters/prometheus v0.53.0
go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.31.0
go.opentelemetry.io/otel/log v0.7.0
go.opentelemetry.io/otel/metric v1.31.0
go.opentelemetry.io/otel/sdk v1.31.0
go.opentelemetry.io/otel/sdk/metric v1.31.0
Expand Down Expand Up @@ -107,7 +109,6 @@ require (
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.31.0 // indirect
go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.7.0 // indirect
go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.31.0 // indirect
go.opentelemetry.io/otel/log v0.7.0 // indirect
go.opentelemetry.io/otel/sdk/log v0.7.0 // indirect
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
golang.org/x/net v0.30.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions service/go.sum
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"fmt"
"runtime"

"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/metric/noop"
sdkresource "go.opentelemetry.io/otel/sdk/resource"
Expand Down Expand Up @@ -92,6 +93,7 @@
telemetrySettings component.TelemetrySettings
host *graph.Host
collectorConf *confmap.Conf
loggerProvider log.LoggerProvider
}

// New creates a new Service, its telemetry, and Components.
Expand Down Expand Up @@ -122,10 +124,11 @@
ZapOptions: set.LoggingOptions,
}

logger, err := telFactory.CreateLogger(ctx, telset, &cfg.Telemetry)
logger, lp, err := telFactory.CreateLogger(ctx, telset, &cfg.Telemetry)
if err != nil {
return nil, fmt.Errorf("failed to create logger: %w", err)
}
srv.loggerProvider = lp

tracerProvider, err := telFactory.CreateTracerProvider(ctx, telset, &cfg.Telemetry)
if err != nil {
Expand Down Expand Up @@ -250,6 +253,12 @@
err = multierr.Append(err, fmt.Errorf("failed to shutdown tracer provider: %w", shutdownErr))
}
}

if prov, ok := srv.loggerProvider.(shutdownable); ok {
if shutdownErr := prov.Shutdown(ctx); shutdownErr != nil {
err = multierr.Append(err, fmt.Errorf("failed to shutdown logger provider: %w", shutdownErr))
}

Check warning on line 260 in service/service.go

View check run for this annotation

Codecov / codecov/patch

service/service.go#L258-L260 

Added lines #L258 - L260 were not covered by tests
}
return err
}

Expand Down
4 changes: 4 additions & 0 deletions service/telemetry/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ type LogsConfig struct {
//
// By default, there is no initial field.
InitialFields map[string]any `mapstructure:"initial_fields"`

// Processors allow configuration of log record processors to emit logs to
// any number of suported backends.
Processors []config.LogRecordProcessor `mapstructure:"processors"`
}

// LogsSamplingConfig sets a sampling strategy for the logger. Sampling caps the
Expand Down
7 changes: 4 additions & 3 deletions service/telemetry/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"go.opentelemetry.io/contrib/config"
"go.opentelemetry.io/otel/log"
"go.opentelemetry.io/otel/metric"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
Expand Down Expand Up @@ -50,7 +51,7 @@ type Factory interface {
CreateDefaultConfig() component.Config

// CreateLogger creates a logger.
CreateLogger(ctx context.Context, set Settings, cfg component.Config) (*zap.Logger, error)
CreateLogger(ctx context.Context, set Settings, cfg component.Config) (*zap.Logger, log.LoggerProvider, error)

// CreateTracerProvider creates a TracerProvider.
CreateTracerProvider(ctx context.Context, set Settings, cfg component.Config) (trace.TracerProvider, error)
Expand All @@ -65,9 +66,9 @@ type Factory interface {
// NewFactory creates a new Factory.
func NewFactory() Factory {
return newFactory(createDefaultConfig,
withLogger(func(_ context.Context, set Settings, cfg component.Config) (*zap.Logger, error) {
withLogger(func(ctx context.Context, set Settings, cfg component.Config) (*zap.Logger, log.LoggerProvider, error) {
c := *cfg.(*Config)
return newLogger(c.Logs, set.ZapOptions)
return newLogger(ctx, set, c)
}),
withTracerProvider(func(ctx context.Context, set Settings, cfg component.Config) (trace.TracerProvider, error) {
c := *cfg.(*Config)
Expand Down
8 changes: 5 additions & 3 deletions service/telemetry/factory_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
import (
"context"

"go.opentelemetry.io/otel/log"
lognoop "go.opentelemetry.io/otel/log/noop"
"go.opentelemetry.io/otel/metric"
metricnoop "go.opentelemetry.io/otel/metric/noop"
"go.opentelemetry.io/otel/trace"
Expand Down Expand Up @@ -45,7 +47,7 @@
}

// createLoggerFunc is the equivalent of Factory.CreateLogger.
type createLoggerFunc func(context.Context, Settings, component.Config) (*zap.Logger, error)
type createLoggerFunc func(context.Context, Settings, component.Config) (*zap.Logger, log.LoggerProvider, error)

// withLogger overrides the default no-op logger.
func withLogger(createLogger createLoggerFunc) factoryOption {
Expand All @@ -54,9 +56,9 @@
})
}

func (f *factory) CreateLogger(ctx context.Context, set Settings, cfg component.Config) (*zap.Logger, error) {
func (f *factory) CreateLogger(ctx context.Context, set Settings, cfg component.Config) (*zap.Logger, log.LoggerProvider, error) {
if f.createLoggerFunc == nil {
return zap.NewNop(), nil
return zap.NewNop(), lognoop.NewLoggerProvider(), nil

Check warning on line 61 in service/telemetry/factory_impl.go

View check run for this annotation

Codecov / codecov/patch

service/telemetry/factory_impl.go#L61 

Added line #L61 was not covered by tests
}
return f.createLoggerFunc(ctx, set, cfg)
}
Expand Down
4 changes: 2 additions & 2 deletions service/telemetry/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestTelemetryConfiguration(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
f := NewFactory()
set := Settings{ZapOptions: []zap.Option{}}
logger, err := f.CreateLogger(context.Background(), set, tt.cfg)
logger, _, err := f.CreateLogger(context.Background(), set, tt.cfg)
if tt.success {
require.NoError(t, err)
assert.NotNil(t, logger)
Expand Down Expand Up @@ -158,7 +158,7 @@ func TestSampledLogger(t *testing.T) {
f := NewFactory()
ctx := context.Background()
set := Settings{ZapOptions: []zap.Option{}}
logger, err := f.CreateLogger(ctx, set, tt.cfg)
logger, _, err := f.CreateLogger(ctx, set, tt.cfg)
require.NoError(t, err)
assert.NotNil(t, logger)
})
Expand Down
73 changes: 59 additions & 14 deletions service/telemetry/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,83 @@
package telemetry // import "go.opentelemetry.io/collector/service/telemetry"

import (
"context"

"go.opentelemetry.io/contrib/bridges/otelzap"

Check failure on line 9 in service/telemetry/logger.go

View workflow job for this annotation

GitHub Actions / contrib-tests-matrix (other) 

missing go.sum entry for module providing package go.opentelemetry.io/contrib/bridges/otelzap (imported by go.opentelemetry.io/collector/service/telemetry); to add:
"go.opentelemetry.io/contrib/config"
"go.opentelemetry.io/otel/log"
semconv "go.opentelemetry.io/otel/semconv/v1.26.0"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)

func newLogger(cfg LogsConfig, options []zap.Option) (*zap.Logger, error) {
func newLogger(ctx context.Context, set Settings, cfg Config) (*zap.Logger, log.LoggerProvider, error) {
// Copied from NewProductionConfig.
zapCfg := &zap.Config{
Level: zap.NewAtomicLevelAt(cfg.Level),
Development: cfg.Development,
Encoding: cfg.Encoding,
Level: zap.NewAtomicLevelAt(cfg.Logs.Level),
Development: cfg.Logs.Development,
Encoding: cfg.Logs.Encoding,
EncoderConfig: zap.NewProductionEncoderConfig(),
OutputPaths: cfg.OutputPaths,
ErrorOutputPaths: cfg.ErrorOutputPaths,
DisableCaller: cfg.DisableCaller,
DisableStacktrace: cfg.DisableStacktrace,
InitialFields: cfg.InitialFields,
OutputPaths: cfg.Logs.OutputPaths,
ErrorOutputPaths: cfg.Logs.ErrorOutputPaths,
DisableCaller: cfg.Logs.DisableCaller,
DisableStacktrace: cfg.Logs.DisableStacktrace,
InitialFields: cfg.Logs.InitialFields,
}

if zapCfg.Encoding == "console" {
// Human-readable timestamps for console format of logs.
zapCfg.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
}

logger, err := zapCfg.Build(options...)
logger, err := zapCfg.Build(set.ZapOptions...)

if err != nil {
return nil, err
return nil, nil, err
}
if cfg.Sampling != nil && cfg.Sampling.Enabled {
logger = newSampledLogger(logger, cfg.Sampling)

var lp log.LoggerProvider

if len(cfg.Logs.Processors) > 0 {
sch := semconv.SchemaURL
res := config.Resource{
SchemaUrl: &sch,
Attributes: attributes(set, cfg),
}
sdk, err := config.NewSDK(
config.WithContext(ctx),
config.WithOpenTelemetryConfiguration(
config.OpenTelemetryConfiguration{
LoggerProvider: &config.LoggerProvider{
Processors: cfg.Logs.Processors,
},
Resource: &res,
},
),
)

if err != nil {
return nil, nil, err
}

lp = sdk.LoggerProvider()

logger = logger.WithOptions(zap.WrapCore(func(c zapcore.Core) zapcore.Core {
return zapcore.NewTee(
Copy link
Member

Choose a reason for hiding this comment

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

Using NewTee here means that logs will be written to both cores right? So logs would be written to stdout and exported? Are we sure we want to do that initially or is configuring a processor acknowledgement that logs won't go to stdout? For users that want the collector to stop writing logs to stdout and only use the bridge to export their logs they won't have a solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i like the idea of having local logs as a fallback, which makes tee'ing nice in that sense. A user could configure the console log exporter if they wanted to have both local logs and data exported to another destination, but the output from the console will be significantly different.

Output from existing logger:

2024-10-22T15:14:38.081-0700    info    [email protected]/service.go:280 Shutdown complete.

Output from console exporter:

{
        "Timestamp": "2024-10-22T15:14:38.081624-07:00",
        "ObservedTimestamp": "2024-10-22T15:14:38.08164-07:00",
        "Severity": 9,
        "SeverityText": "info",
        "Body": {
                "Type": "String",
                "Value": "Shutdown complete."
        },
        "Attributes": [],
        "TraceID": "00000000000000000000000000000000",
        "SpanID": "0000000000000000",
        "TraceFlags": "00",
        "Resource": [
                {
                        "Key": "service.name",
                        "Value": {
                                "Type": "STRING",
                                "Value": "otelcorecol"
                        }
                },
                {
                        "Key": "service.version",
                        "Value": {
                                "Type": "STRING",
                                "Value": "0.111.0-dev"
                        }
                },
                {
                        "Key": "telemetry.sdk.language",
                        "Value": {
                                "Type": "STRING",
                                "Value": "go"
                        }
                },
                {
                        "Key": "telemetry.sdk.name",
                        "Value": {
                                "Type": "STRING",
                                "Value": "opentelemetry"
                        }
                },
                {
                        "Key": "telemetry.sdk.version",
                        "Value": {
                                "Type": "STRING",
                                "Value": "1.31.0"
                        }
                }
        ],
        "Scope": {
                "Name": "go.opentelemetry.io/collector/service/telemetry",
                "Version": "",
                "SchemaURL": ""
        },
        "DroppedAttributes": 0
}

Copy link
Member

Choose a reason for hiding this comment

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

One case where I think the fallback is interesting is if the OTLP logs exporting fails and we have some log from e.g. grpc-go that would be helpful in debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR gives users the option to start producing logs over OTLP, without breaking existing functionality. I would suggest we follow up this PR with a feature gate that disables the tee'ing of the logs.

This would give users that don't want to double emit the logs to turn off the local console logging if a provider is configured. This could become the default in the future if users support this idea.

One thing to note is that configuration of the logging today supports sampling logs, which the otel logging provider has no concept of at this time

Copy link
Member

Choose a reason for hiding this comment

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

That works for me. As long as we have some plan for how to give users the option to disable the console logging if they want I'm happy.

c,
otelzap.NewCore("go.opentelemetry.io/collector/service/telemetry",
otelzap.WithLoggerProvider(lp),
),
)
}))

}

if cfg.Logs.Sampling != nil && cfg.Logs.Sampling.Enabled {
logger = newSampledLogger(logger, cfg.Logs.Sampling)
}

return logger, nil
return logger, lp, nil
}

func newSampledLogger(logger *zap.Logger, sc *LogsSamplingConfig) *zap.Logger {
Expand Down
Loading
Loading