From cb7a318347983a747c6e1fa45640e341b0e07a10 Mon Sep 17 00:00:00 2001 From: Munir Abdinur Date: Fri, 2 Aug 2024 19:43:14 -0400 Subject: [PATCH] address comments --- pkg/otlp/metrics/config.go | 21 +++++++++++---------- pkg/otlp/metrics/kafka_mapping.go | 2 +- pkg/otlp/metrics/metrics_remapping.go | 15 ++++++++------- pkg/otlp/metrics/metrics_translator.go | 6 ++---- pkg/otlp/metrics/metrics_translator_test.go | 6 +++--- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/pkg/otlp/metrics/config.go b/pkg/otlp/metrics/config.go index 2c2b7277..69b28f58 100644 --- a/pkg/otlp/metrics/config.go +++ b/pkg/otlp/metrics/config.go @@ -34,14 +34,13 @@ type translatorConfig struct { // withRemapping reports whether certain metrics that are only available when using // the Datadog Agent should be obtained by remapping from OTEL counterparts (e.g. - // container.* and system.* metrics). + // container.* and system.* metrics). This configuration also enables withOTelPrefix. withRemapping bool - // withRenaming reports whether OpenTelemetry system and process metrics should be - // renamed with the `otel.` prefix. Enabling this configuration disables computing - // Datadog metrics from OpenTelemetry metrics. This is useful when the translator is - // running as part of a Collector with the Datadog Agent. - withRenaming bool + // withOTelPrefix reports whether some OpenTelemetry metrics (ex: host metrics) should be + // renamed with the `otel.` prefix. This prevents the Collector and Datadog + // Agent from computing metrics with the same names. + withOTelPrefix bool // cache configuration sweepInterval int64 @@ -64,16 +63,18 @@ type TranslatorOption func(*translatorConfig) error func WithRemapping() TranslatorOption { return func(t *translatorConfig) error { t.withRemapping = true + // to maintain backward compatibility with the old remapping logic + // withRemapping must rename some otel metrics + t.withOTelPrefix = true return nil } } -// withRenaming appends the `otel.` prefix to otel system and process metrics. -// Also disables the remapping of OpenTelemetry metrics to Datadog metrics. T +// withOTelPrefix appends the `otel.` prefix to OpenTelemetry metrics. // This is useful when the translator is running as part of a Collector with the Datadog Agent. -func WithRenaming() TranslatorOption { +func WithOTelPrefix() TranslatorOption { return func(t *translatorConfig) error { - t.withRenaming = true + t.withOTelPrefix = true return nil } } diff --git a/pkg/otlp/metrics/kafka_mapping.go b/pkg/otlp/metrics/kafka_mapping.go index 6f042355..550174a7 100644 --- a/pkg/otlp/metrics/kafka_mapping.go +++ b/pkg/otlp/metrics/kafka_mapping.go @@ -428,7 +428,7 @@ func remapJvmMetrics(all pmetric.MetricSlice, m pmetric.Metric) { } } -// renameKafkaMetrics renames otel kafka metrics to avoid conflicts with DD agent metrics +// renameKafkaMetrics renames otel kafka metrics to avoid conflicts with DD agent metrics. func renameKafkaMetrics(m pmetric.Metric) { switch m.Name() { case "kafka.producer.request-rate", "kafka.producer.response-rate", "kafka.producer.request-latency-avg", "kafka.consumer.fetch-size-avg", "kafka.producer.compression-rate", "kafka.producer.record-retry-rate", "kafka.producer.record-send-rate", "kafka.producer.record-error-rate": diff --git a/pkg/otlp/metrics/metrics_remapping.go b/pkg/otlp/metrics/metrics_remapping.go index 5c29df1a..6c8b8896 100644 --- a/pkg/otlp/metrics/metrics_remapping.go +++ b/pkg/otlp/metrics/metrics_remapping.go @@ -37,15 +37,16 @@ func remapMetrics(all pmetric.MetricSlice, m pmetric.Metric) { remapJvmMetrics(all, m) } +// renameMetrics adds the `otel.` prefix to metrics. func renameMetrics(m pmetric.Metric) { - renameSystemMetrics(m) + renameHostMetrics(m) renameKafkaMetrics(m) } // remapSystemMetrics extracts system metrics from m and appends them to all. func remapSystemMetrics(all pmetric.MetricSlice, m pmetric.Metric) { name := m.Name() - if !isSystemMetric(name) { + if !isHostMetric(name) { return } switch name { @@ -132,8 +133,8 @@ func remapContainerMetrics(all pmetric.MetricSlice, m pmetric.Metric) { } } -// isSystemMetric determines whether a metric is a system metric. -func isSystemMetric(name string) bool { +// isHostMetric determines whether a metric is a system metric. +func isHostMetric(name string) bool { return strings.HasPrefix(name, "process.") || strings.HasPrefix(name, "system.") } @@ -230,9 +231,9 @@ func hasAny(point pmetric.NumberDataPoint, tags ...kv) bool { return false } -// renameSystemMetrics renames otel system metrics to avoid conflicts with DD agent metrics -func renameSystemMetrics(m pmetric.Metric) { - if isSystemMetric(m.Name()) { +// renameHostMetrics renames otel host metrics to avoid conflicts with DD agent metrics. +func renameHostMetrics(m pmetric.Metric) { + if isHostMetric(m.Name()) { m.SetName("otel." + m.Name()) } } diff --git a/pkg/otlp/metrics/metrics_translator.go b/pkg/otlp/metrics/metrics_translator.go index f02c9d59..de3b670d 100644 --- a/pkg/otlp/metrics/metrics_translator.go +++ b/pkg/otlp/metrics/metrics_translator.go @@ -805,10 +805,8 @@ func (t *Translator) MapMetrics(ctx context.Context, md pmetric.Metrics, consume if t.cfg.withRemapping { remapMetrics(newMetrics, md) - // to maintain backward compatibility with the old remapping logic - // we also need to rename some otel metrics - renameMetrics(md) - } else if t.cfg.withRenaming { + } + if t.cfg.withOTelPrefix { renameMetrics(md) } diff --git a/pkg/otlp/metrics/metrics_translator_test.go b/pkg/otlp/metrics/metrics_translator_test.go index dd7360bf..52b0bd46 100644 --- a/pkg/otlp/metrics/metrics_translator_test.go +++ b/pkg/otlp/metrics/metrics_translator_test.go @@ -1018,10 +1018,10 @@ func TestMapRuntimeMetricsHasMappingCollector(t *testing.T) { assert.Equal(t, []string{"go"}, rmt.Languages) } -func TestMapSystemMetricsRenamedWithRenaming(t *testing.T) { +func TestMapSystemMetricsRenamedWithOTelPrefix(t *testing.T) { ctx := context.Background() - // WithRenaming() is used to rename the system metrics, this overrides WithRemapping. - tr := NewTestTranslator(t, WithRenaming()) + // WithOTelPrefix() is used to rename the system metrics, this overrides WithRemapping. + tr := NewTestTranslator(t, WithOTelPrefix()) consumer := &mockFullConsumer{} systemDims := newDims("system.cpu.utilization") processDims := newDims("process.runtime.go.goroutines")