From aebd396163f5a24e39b597b9c11c2a3360bc30fa Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 26 Jun 2024 20:50:51 +0800 Subject: [PATCH 1/3] [exporter/elasticsearch] deprecate 'dedup' config --- .chloggen/elasticsearch-deprecate-dedup.yaml | 27 ++++++++++++++++++++ exporter/elasticsearchexporter/README.md | 8 +++--- exporter/elasticsearchexporter/config.go | 4 +++ 3 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 .chloggen/elasticsearch-deprecate-dedup.yaml diff --git a/.chloggen/elasticsearch-deprecate-dedup.yaml b/.chloggen/elasticsearch-deprecate-dedup.yaml new file mode 100644 index 000000000000..d9f815737647 --- /dev/null +++ b/.chloggen/elasticsearch-deprecate-dedup.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: deprecation + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: exporter/elasticsearch + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Deprecate the "dedup" configuration. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [33773] + +# (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: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# 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/exporter/elasticsearchexporter/README.md b/exporter/elasticsearchexporter/README.md index 8b52bf4da343..07a9425812bd 100644 --- a/exporter/elasticsearchexporter/README.md +++ b/exporter/elasticsearchexporter/README.md @@ -127,10 +127,10 @@ behaviours, which may be configured throug the following settings: field names for span events. - `fields` (optional): Configure additional fields mappings. - `file` (optional): Read additional field mappings from the provided YAML file. - - `dedup` (default=true): Try to find and remove duplicate fields/attributes - from events before publishing to Elasticsearch. Some structured logging - libraries can produce duplicate fields (for example zap). Elasticsearch - will reject documents that have duplicate fields. + - `dedup` (default=true; DEPRECATED, in future deduplication will always be enabled): + Try to find and remove duplicate fields/attributes from events before publishing + to Elasticsearch. Some structured logging libraries can produce duplicate fields + (for example zap). Elasticsearch will reject documents that have duplicate fields. - `dedot` (default=true): When enabled attributes with `.` will be split into proper json objects. diff --git a/exporter/elasticsearchexporter/config.go b/exporter/elasticsearchexporter/config.go index daf83e4da3ba..bebc16235367 100644 --- a/exporter/elasticsearchexporter/config.go +++ b/exporter/elasticsearchexporter/config.go @@ -157,6 +157,10 @@ type MappingsSettings struct { File string `mapstructure:"file"` // Try to find and remove duplicate fields + // + // Deprecated: [v0.104.0] deduplication will always be applied in future, + // with no option to disable. Disabling deduplication is not meaningful, + // as Elasticsearch will reject documents with duplicate JSON object keys. Dedup bool `mapstructure:"dedup"` Dedot bool `mapstructure:"dedot"` From 62f946cbc5aa32fdc415fe621b3b1f67e1a17807 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 27 Jun 2024 10:06:09 +0800 Subject: [PATCH 2/3] Log deprecation warning --- exporter/elasticsearchexporter/config.go | 7 ++++ exporter/elasticsearchexporter/factory.go | 3 ++ .../elasticsearchexporter/factory_test.go | 32 +++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/exporter/elasticsearchexporter/config.go b/exporter/elasticsearchexporter/config.go index bebc16235367..4d0907777677 100644 --- a/exporter/elasticsearchexporter/config.go +++ b/exporter/elasticsearchexporter/config.go @@ -15,6 +15,7 @@ import ( "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/configopaque" "go.opentelemetry.io/collector/exporter/exporterhelper" + "go.uber.org/zap" ) // Config defines configuration for Elastic exporter. @@ -313,3 +314,9 @@ func parseCloudID(input string) (*url.URL, error) { func (cfg *Config) MappingMode() MappingMode { return mappingModes[cfg.Mapping.Mode] } + +func logConfigDeprecationWarnings(cfg *Config, logger *zap.Logger) { + if !cfg.Mapping.Dedup { + logger.Warn("dedup has been deprecated, and will always be enabled in future") + } +} diff --git a/exporter/elasticsearchexporter/factory.go b/exporter/elasticsearchexporter/factory.go index 8829215c8ac6..cc2e27421f39 100644 --- a/exporter/elasticsearchexporter/factory.go +++ b/exporter/elasticsearchexporter/factory.go @@ -92,6 +92,7 @@ func createLogsExporter( set.Logger.Warn("index option are deprecated and replaced with logs_index and traces_index.") index = cf.Index } + logConfigDeprecationWarnings(cf, set.Logger) exporter, err := newExporter(cf, set, index, cf.LogsDynamicIndex.Enabled) if err != nil { @@ -115,6 +116,7 @@ func createMetricsExporter( cfg component.Config, ) (exporter.Metrics, error) { cf := cfg.(*Config) + logConfigDeprecationWarnings(cf, set.Logger) exporter, err := newExporter(cf, set, cf.MetricsIndex, cf.MetricsDynamicIndex.Enabled) if err != nil { @@ -136,6 +138,7 @@ func createTracesExporter(ctx context.Context, cfg component.Config) (exporter.Traces, error) { cf := cfg.(*Config) + logConfigDeprecationWarnings(cf, set.Logger) exporter, err := newExporter(cf, set, cf.TracesIndex, cf.TracesDynamicIndex.Enabled) if err != nil { diff --git a/exporter/elasticsearchexporter/factory_test.go b/exporter/elasticsearchexporter/factory_test.go index 4ee3f568593d..2a3b26d3ac65 100644 --- a/exporter/elasticsearchexporter/factory_test.go +++ b/exporter/elasticsearchexporter/factory_test.go @@ -11,6 +11,8 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/exporter/exportertest" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" ) func TestCreateDefaultConfig(t *testing.T) { @@ -94,3 +96,33 @@ func TestFactory_CreateLogsAndTracesExporterWithDeprecatedIndexOption(t *testing require.NotNil(t, tracesExporter) require.NoError(t, tracesExporter.Shutdown(context.Background())) } + +func TestFactory_DedupDeprecated(t *testing.T) { + factory := NewFactory() + cfg := withDefaultConfig(func(cfg *Config) { + cfg.Endpoint = "http://testing.invalid:9200" + cfg.Mapping.Dedup = false + }) + + loggerCore, logObserver := observer.New(zap.WarnLevel) + set := exportertest.NewNopSettings() + set.Logger = zap.New(loggerCore) + + logsExporter, err := factory.CreateLogsExporter(context.Background(), set, cfg) + require.NoError(t, err) + require.NoError(t, logsExporter.Shutdown(context.Background())) + + tracesExporter, err := factory.CreateTracesExporter(context.Background(), set, cfg) + require.NoError(t, err) + require.NoError(t, tracesExporter.Shutdown(context.Background())) + + metricsExporter, err := factory.CreateMetricsExporter(context.Background(), set, cfg) + require.NoError(t, err) + require.NoError(t, metricsExporter.Shutdown(context.Background())) + + records := logObserver.AllUntimed() + assert.Len(t, records, 3) + assert.Equal(t, "dedup has been deprecated, and will always be enabled in future", records[0].Message) + assert.Equal(t, "dedup has been deprecated, and will always be enabled in future", records[1].Message) + assert.Equal(t, "dedup has been deprecated, and will always be enabled in future", records[2].Message) +} From 7c4ec67980c3d6c064ac26b5b5206fa81f9247d9 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 27 Jun 2024 13:43:43 +0800 Subject: [PATCH 3/3] Update .chloggen/elasticsearch-deprecate-dedup.yaml --- .chloggen/elasticsearch-deprecate-dedup.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/elasticsearch-deprecate-dedup.yaml b/.chloggen/elasticsearch-deprecate-dedup.yaml index d9f815737647..47726a48b729 100644 --- a/.chloggen/elasticsearch-deprecate-dedup.yaml +++ b/.chloggen/elasticsearch-deprecate-dedup.yaml @@ -15,7 +15,7 @@ issues: [33773] # (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: +subtext: dedup has been deprecated, and will always be enabled in future. # If your change doesn't affect end users or the exported elements of any package, # you should instead start your pull request title with [chore] or use the "Skip Changelog" label.