From fbebbec1a277975cab0752ba459b21b4e5fa4815 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Sun, 11 May 2025 20:29:31 -0700 Subject: [PATCH 1/2] Avoid allocating too much memory, sampling logic is not re-applied Signed-off-by: Bogdan Drutu --- .chloggen/fix-13014.yaml | 25 +++++++++++++ .../componentattribute/logger_test.go | 2 +- .../componentattribute/logger_zap.go | 37 +++---------------- service/telemetry/logger.go | 22 +++++++---- service/telemetry/logger_test.go | 2 +- 5 files changed, 46 insertions(+), 42 deletions(-) create mode 100644 .chloggen/fix-13014.yaml diff --git a/.chloggen/fix-13014.yaml b/.chloggen/fix-13014.yaml new file mode 100644 index 00000000000..f241b6ba26c --- /dev/null +++ b/.chloggen/fix-13014.yaml @@ -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: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: telemetry + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Avoid allocating too much memory, sampling logic is not re-applied + +# One or more tracking issues or pull requests related to the change +issues: [13014] + +# (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: [user] diff --git a/internal/telemetry/componentattribute/logger_test.go b/internal/telemetry/componentattribute/logger_test.go index 95127b05934..e38d04ad085 100644 --- a/internal/telemetry/componentattribute/logger_test.go +++ b/internal/telemetry/componentattribute/logger_test.go @@ -90,7 +90,7 @@ func TestCore(t *testing.T) { createLogger: func() (*zap.Logger, logRecorder) { core, observed := createZapCore() recorder := logtest.NewRecorder() - core = componentattribute.NewOTelTeeCoreWithAttributes(core, recorder, "testinstr", zap.DebugLevel, attribute.NewSet()) + core = componentattribute.NewOTelTeeCoreWithAttributes(core, recorder, "testinstr", zap.DebugLevel, attribute.NewSet(), func(c zapcore.Core) zapcore.Core { return c }) return zap.New(core), logRecorder{zapLogs: observed, otelLogs: recorder} }, check: func(t *testing.T, rec logRecorder) { diff --git a/internal/telemetry/componentattribute/logger_zap.go b/internal/telemetry/componentattribute/logger_zap.go index e20a4e5fd68..282335235dd 100644 --- a/internal/telemetry/componentattribute/logger_zap.go +++ b/internal/telemetry/componentattribute/logger_zap.go @@ -76,6 +76,7 @@ type otelTeeCoreWithAttributes struct { lp log.LoggerProvider scopeName string level zapcore.Level + wrapper func(zapcore.Core) zapcore.Core } var _ coreWithAttributes = (*otelTeeCoreWithAttributes)(nil) @@ -84,7 +85,7 @@ var _ coreWithAttributes = (*otelTeeCoreWithAttributes)(nil) // logs, component attributes are injected as instrumentation scope attributes. // // This is used when service::telemetry::logs::processors is configured. -func NewOTelTeeCoreWithAttributes(consoleCore zapcore.Core, lp log.LoggerProvider, scopeName string, level zapcore.Level, attrs attribute.Set) zapcore.Core { +func NewOTelTeeCoreWithAttributes(consoleCore zapcore.Core, lp log.LoggerProvider, scopeName string, level zapcore.Level, attrs attribute.Set, wrapper func(zapcore.Core) zapcore.Core) zapcore.Core { // TODO: Use `otelzap.WithAttributes` and remove `LoggerProviderWithAttributes` // once we've upgraded to otelzap v0.11.0. lpwa := LoggerProviderWithAttributes(lp, attrs) @@ -97,45 +98,17 @@ func NewOTelTeeCoreWithAttributes(consoleCore zapcore.Core, lp log.LoggerProvide } return &otelTeeCoreWithAttributes{ - Core: zapcore.NewTee(consoleCore, otelCore), + Core: zapcore.NewTee(consoleCore, wrapper(otelCore)), consoleCore: consoleCore, lp: lp, scopeName: scopeName, level: level, + wrapper: wrapper, } } func (ocwa *otelTeeCoreWithAttributes) withAttributeSet(attrs attribute.Set) zapcore.Core { - return NewOTelTeeCoreWithAttributes( - tryWithAttributeSet(ocwa.consoleCore, attrs), - ocwa.lp, ocwa.scopeName, ocwa.level, - attrs, - ) -} - -type wrapperCoreWithAttributes struct { - zapcore.Core - from zapcore.Core - wrapper func(zapcore.Core) zapcore.Core -} - -var _ coreWithAttributes = (*wrapperCoreWithAttributes)(nil) - -// NewWrapperCoreWithAttributes applies a wrapper function to a core, similar to [zap.WrapCore]. The resulting wrapped core -// allows setting component attributes on the inner core and reapplying the wrapper function when -// needed. -// -// This is used when adding [zapcore.NewSamplerWithOptions] to our logger stack. -func NewWrapperCoreWithAttributes(from zapcore.Core, wrapper func(zapcore.Core) zapcore.Core) zapcore.Core { - return &wrapperCoreWithAttributes{ - Core: wrapper(from), - from: from, - wrapper: wrapper, - } -} - -func (wcwa *wrapperCoreWithAttributes) withAttributeSet(attrs attribute.Set) zapcore.Core { - return NewWrapperCoreWithAttributes(tryWithAttributeSet(wcwa.from, attrs), wcwa.wrapper) + return NewOTelTeeCoreWithAttributes(tryWithAttributeSet(ocwa.consoleCore, attrs), ocwa.lp, ocwa.scopeName, ocwa.level, attrs, ocwa.wrapper) } // ZapLoggerWithAttributes creates a Zap Logger with a new set of injected component attributes. diff --git a/service/telemetry/logger.go b/service/telemetry/logger.go index 82984e2a18e..08d8dd65300 100644 --- a/service/telemetry/logger.go +++ b/service/telemetry/logger.go @@ -44,7 +44,7 @@ func newLogger(set Settings, cfg Config) (*zap.Logger, log.LoggerProvider, error // We do NOT add them to the logger using With, because that would apply to all logs, even ones exported through the core that wraps // the LoggerProvider, meaning that the attributes would be exported twice. logger = logger.WithOptions(zap.WrapCore(func(c zapcore.Core) zapcore.Core { - fields := []zap.Field{} + var fields []zap.Field for k, v := range cfg.Resource { if v != nil { f := zap.Stringp(k, v) @@ -56,12 +56,23 @@ func newLogger(set Settings, cfg Config) (*zap.Logger, log.LoggerProvider, error })) var lp log.LoggerProvider - logger = logger.WithOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core { + if cfg.Logs.Sampling != nil && cfg.Logs.Sampling.Enabled { + core = newSampledCore(core, cfg.Logs.Sampling) + } + core = componentattribute.NewConsoleCoreWithAttributes(core, attribute.NewSet()) if len(cfg.Logs.Processors) > 0 && set.SDK != nil { lp = set.SDK.LoggerProvider() + wrapper := func(c zapcore.Core) zapcore.Core { + return c + } + if cfg.Logs.Sampling != nil && cfg.Logs.Sampling.Enabled { + wrapper = func(c zapcore.Core) zapcore.Core { + return newSampledCore(c, cfg.Logs.Sampling) + } + } core = componentattribute.NewOTelTeeCoreWithAttributes( core, @@ -69,15 +80,10 @@ func newLogger(set Settings, cfg Config) (*zap.Logger, log.LoggerProvider, error "go.opentelemetry.io/collector/service/telemetry", cfg.Logs.Level, attribute.NewSet(), + wrapper, ) } - if cfg.Logs.Sampling != nil && cfg.Logs.Sampling.Enabled { - core = componentattribute.NewWrapperCoreWithAttributes(core, func(c zapcore.Core) zapcore.Core { - return newSampledCore(c, cfg.Logs.Sampling) - }) - } - return core })) diff --git a/service/telemetry/logger_test.go b/service/telemetry/logger_test.go index 9fd1dfa31c1..8c80db2a8a9 100644 --- a/service/telemetry/logger_test.go +++ b/service/telemetry/logger_test.go @@ -98,7 +98,7 @@ func TestNewLogger(t *testing.T) { InitialFields: map[string]any(nil), }, }, - wantCoreType: "*componentattribute.wrapperCoreWithAttributes", + wantCoreType: "*componentattribute.consoleCoreWithAttributes", }, } for _, tt := range tests { From 9af91904489022a23667e8aed601e76baa763413 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Mon, 12 May 2025 09:07:58 -0700 Subject: [PATCH 2/2] Update .chloggen/fix-13014.yaml Co-authored-by: Jade Guiton --- .chloggen/fix-13014.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/fix-13014.yaml b/.chloggen/fix-13014.yaml index f241b6ba26c..bb7d8b0411f 100644 --- a/.chloggen/fix-13014.yaml +++ b/.chloggen/fix-13014.yaml @@ -7,7 +7,7 @@ change_type: bug_fix component: telemetry # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Avoid allocating too much memory, sampling logic is not re-applied +note: Allocate less memory per component when OTLP exporting of logs is disabled # One or more tracking issues or pull requests related to the change issues: [13014]