From df1967ab2f880c6718b28cbe7f6b4a5f6218ecba Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Thu, 10 Nov 2022 21:31:23 +0800 Subject: [PATCH] Signed-off-by: Ziqi Zhao prevent conflict metric info --- CHANGELOG.md | 1 + exporters/prometheus/exporter.go | 78 ++++++++++-- exporters/prometheus/exporter_test.go | 119 ++++++++++++++++++ exporters/prometheus/go.mod | 4 +- .../prometheus/testdata/conflict_help.txt | 11 ++ .../testdata/conflict_type_counter.txt | 9 ++ .../prometheus/testdata/conflict_unit.txt | 11 ++ exporters/prometheus/testdata/no_conflict.txt | 11 ++ 8 files changed, 234 insertions(+), 10 deletions(-) create mode 100644 exporters/prometheus/testdata/conflict_help.txt create mode 100644 exporters/prometheus/testdata/conflict_type_counter.txt create mode 100644 exporters/prometheus/testdata/conflict_unit.txt create mode 100644 exporters/prometheus/testdata/no_conflict.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index ac0bdc06da8..6d6f004c4b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Asynchronous callbacks are only called if they are registered with at least one instrument that does not use drop aggragation. (#3408) - Do not report empty partial-success responses in the `go.opentelemetry.io/otel/exporters/otlp` exporters. (#3438, #3432) - Handle partial success responses in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric` exporters. (#3162, #3440) +- Prevent duplicate Prometheus description, unit, and type. (#3469) ## [1.11.1/0.33.0] 2022-10-19 diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 2cb23cc19fb..8cc87573db9 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -24,6 +24,8 @@ import ( "unicode/utf8" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "google.golang.org/protobuf/proto" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -62,6 +64,7 @@ type collector struct { disableScopeInfo bool createTargetInfoOnce sync.Once scopeInfos map[instrumentation.Scope]prometheus.Metric + metricFamilies map[string]*dto.MetricFamily } // prometheus counters MUST have a _total suffix: @@ -83,6 +86,7 @@ func New(opts ...Option) (*Exporter, error) { withoutUnits: cfg.withoutUnits, disableScopeInfo: cfg.disableScopeInfo, scopeInfos: make(map[instrumentation.Scope]prometheus.Metric), + metricFamilies: make(map[string]*dto.MetricFamily), } if err := cfg.registerer.Register(collector); err != nil { @@ -149,22 +153,32 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { for _, m := range scopeMetrics.Metrics { switch v := m.Data.(type) { case metricdata.Histogram: - addHistogramMetric(ch, v, m, keys, values, c.getName(m)) + addHistogramMetric(ch, v, m, keys, values, c.getName(m), c.metricFamilies) case metricdata.Sum[int64]: - addSumMetric(ch, v, m, keys, values, c.getName(m)) + addSumMetric(ch, v, m, keys, values, c.getName(m), c.metricFamilies) case metricdata.Sum[float64]: - addSumMetric(ch, v, m, keys, values, c.getName(m)) + addSumMetric(ch, v, m, keys, values, c.getName(m), c.metricFamilies) case metricdata.Gauge[int64]: - addGaugeMetric(ch, v, m, keys, values, c.getName(m)) + addGaugeMetric(ch, v, m, keys, values, c.getName(m), c.metricFamilies) case metricdata.Gauge[float64]: - addGaugeMetric(ch, v, m, keys, values, c.getName(m)) + addGaugeMetric(ch, v, m, keys, values, c.getName(m), c.metricFamilies) } } } } -func addHistogramMetric(ch chan<- prometheus.Metric, histogram metricdata.Histogram, m metricdata.Metrics, ks, vs [2]string, name string) { +func addHistogramMetric(ch chan<- prometheus.Metric, histogram metricdata.Histogram, m metricdata.Metrics, ks, vs [2]string, name string, mfs map[string]*dto.MetricFamily) { // TODO(https://github.com/open-telemetry/opentelemetry-go/issues/3163): support exemplars + drop, help := validateMetrics(name, m.Description, dto.MetricType_HISTOGRAM.Enum(), mfs) + if drop { + otel.Handle(fmt.Errorf("type conflict")) + return + } + if help != "" { + otel.Handle(fmt.Errorf("help conflict")) + m.Description = help + } + for _, dp := range histogram.DataPoints { keys, values := getAttrs(dp.Attributes, ks, vs) @@ -185,15 +199,28 @@ func addHistogramMetric(ch chan<- prometheus.Metric, histogram metricdata.Histog } } -func addSumMetric[N int64 | float64](ch chan<- prometheus.Metric, sum metricdata.Sum[N], m metricdata.Metrics, ks, vs [2]string, name string) { +func addSumMetric[N int64 | float64](ch chan<- prometheus.Metric, sum metricdata.Sum[N], m metricdata.Metrics, ks, vs [2]string, name string, mfs map[string]*dto.MetricFamily) { valueType := prometheus.CounterValue + metricType := dto.MetricType_COUNTER if !sum.IsMonotonic { valueType = prometheus.GaugeValue + metricType = dto.MetricType_GAUGE } if sum.IsMonotonic { // Add _total suffix for counters name += counterSuffix } + + drop, help := validateMetrics(name, m.Description, metricType.Enum(), mfs) + if drop { + otel.Handle(fmt.Errorf("type conflict")) + return + } + if help != "" { + otel.Handle(fmt.Errorf("help conflict")) + m.Description = help + } + for _, dp := range sum.DataPoints { keys, values := getAttrs(dp.Attributes, ks, vs) @@ -207,7 +234,17 @@ func addSumMetric[N int64 | float64](ch chan<- prometheus.Metric, sum metricdata } } -func addGaugeMetric[N int64 | float64](ch chan<- prometheus.Metric, gauge metricdata.Gauge[N], m metricdata.Metrics, ks, vs [2]string, name string) { +func addGaugeMetric[N int64 | float64](ch chan<- prometheus.Metric, gauge metricdata.Gauge[N], m metricdata.Metrics, ks, vs [2]string, name string, mfs map[string]*dto.MetricFamily) { + drop, help := validateMetrics(name, m.Description, dto.MetricType_GAUGE.Enum(), mfs) + if drop { + otel.Handle(fmt.Errorf("type conflict")) + return + } + if help != "" { + otel.Handle(fmt.Errorf("help conflict")) + m.Description = help + } + for _, dp := range gauge.DataPoints { keys, values := getAttrs(dp.Attributes, ks, vs) @@ -344,3 +381,28 @@ func sanitizeName(n string) string { return b.String() } + +func validateMetrics(name, description string, metricType *dto.MetricType, mfs map[string]*dto.MetricFamily) (drop bool, help string) { + emf, exist := mfs[name] + if !exist { + mfs[name] = &dto.MetricFamily{ + Name: proto.String(name), + Help: proto.String(description), + Type: metricType, + } + return false, "" + } + + if emf.GetHelp() != description { + if emf.GetType() == dto.MetricType_HISTOGRAM || *metricType == dto.MetricType_HISTOGRAM { + return false, "" + } + return false, emf.GetHelp() + } + + if emf.GetType() != *metricType { + return true, "" + } + + return false, "" +} diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 4edf11ba48a..33e58268d6a 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -386,3 +386,122 @@ func TestMultiScopes(t *testing.T) { err = testutil.GatherAndCompare(registry, file) require.NoError(t, err) } + +func TestDuplicateMetrics(t *testing.T) { + testCases := []struct { + name string + customResouceAttrs []attribute.KeyValue + recordMetrics func(ctx context.Context, meterA, meterB otelmetric.Meter) + options []Option + expectedFile string + }{ + { + name: "no_conflict", + recordMetrics: func(ctx context.Context, meterA, meterB otelmetric.Meter) { + fooA, err := meterA.SyncInt64().Counter("foo", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foo")) + assert.NoError(t, err) + fooA.Add(ctx, 100, attribute.String("type", "foo")) + + fooB, err := meterB.SyncInt64().Counter("foo", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foo")) + assert.NoError(t, err) + fooB.Add(ctx, 100, attribute.String("type", "foo")) + }, + expectedFile: "testdata/no_conflict.txt", + }, + { + name: "conflict_help", + recordMetrics: func(ctx context.Context, meterA, meterB otelmetric.Meter) { + barA, err := meterA.SyncInt64().Counter("bar", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter a bar")) + assert.NoError(t, err) + barA.Add(ctx, 100, attribute.String("type", "bar")) + + barB, err := meterB.SyncInt64().Counter("bar", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter b bar")) + assert.NoError(t, err) + barB.Add(ctx, 100, attribute.String("type", "bar")) + }, + expectedFile: "testdata/conflict_help.txt", + }, + { + name: "conflict_unit", + recordMetrics: func(ctx context.Context, meterA, meterB otelmetric.Meter) { + bazA, err := meterA.SyncInt64().Counter("baz", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter baz")) + assert.NoError(t, err) + bazA.Add(ctx, 100, attribute.String("type", "baz")) + + bazB, err := meterB.SyncInt64().Counter("baz", + instrument.WithUnit(unit.Milliseconds), + instrument.WithDescription("meter baz")) + assert.NoError(t, err) + bazB.Add(ctx, 100, attribute.String("type", "baz")) + }, + options: []Option{WithoutUnits()}, + expectedFile: "testdata/conflict_unit.txt", + }, + { + name: "conflict_type_counter", + recordMetrics: func(ctx context.Context, meterA, meterB otelmetric.Meter) { + counter, err := meterA.SyncInt64().Counter("foz", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foz")) + assert.NoError(t, err) + counter.Add(ctx, 100, attribute.String("type", "foz")) + + gauge, err := meterA.SyncInt64().UpDownCounter("foz_total", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foz")) + assert.NoError(t, err) + gauge.Add(ctx, 200, attribute.String("type", "foz")) + }, + options: []Option{WithoutUnits()}, + expectedFile: "testdata/conflict_type_counter.txt", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // initialize registry exporter + ctx := context.Background() + registry := prometheus.NewRegistry() + exporter, err := New(append(tc.options, WithRegisterer(registry))...) + require.NoError(t, err) + + // initialize resource + res, err := resource.New(ctx, + resource.WithAttributes(semconv.ServiceNameKey.String("prometheus_test")), + resource.WithAttributes(semconv.TelemetrySDKVersionKey.String("latest")), + ) + require.NoError(t, err) + res, err = resource.Merge(resource.Default(), res) + require.NoError(t, err) + + // initialize provider + provider := metric.NewMeterProvider( + metric.WithReader(exporter), + metric.WithResource(res), + ) + + // initialize two meter a, b + meterA := provider.Meter("ma", otelmetric.WithInstrumentationVersion("v0.1.0")) + meterB := provider.Meter("mb", otelmetric.WithInstrumentationVersion("v0.1.0")) + + tc.recordMetrics(ctx, meterA, meterB) + + file, err := os.Open(tc.expectedFile) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, file.Close()) }) + + err = testutil.GatherAndCompare(registry, file) + require.NoError(t, err) + }) + } +} diff --git a/exporters/prometheus/go.mod b/exporters/prometheus/go.mod index 731be2dfd70..33743742181 100644 --- a/exporters/prometheus/go.mod +++ b/exporters/prometheus/go.mod @@ -4,11 +4,13 @@ go 1.18 require ( github.com/prometheus/client_golang v1.14.0 + github.com/prometheus/client_model v0.3.0 github.com/stretchr/testify v1.8.1 go.opentelemetry.io/otel v1.11.1 go.opentelemetry.io/otel/metric v0.33.0 go.opentelemetry.io/otel/sdk v1.11.1 go.opentelemetry.io/otel/sdk/metric v0.33.0 + google.golang.org/protobuf v1.28.1 ) require ( @@ -20,12 +22,10 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/prometheus/client_model v0.3.0 // indirect github.com/prometheus/common v0.37.0 // indirect github.com/prometheus/procfs v0.8.0 // indirect go.opentelemetry.io/otel/trace v1.11.1 // indirect golang.org/x/sys v0.0.0-20220919091848-fb04ddd9f9c8 // indirect - google.golang.org/protobuf v1.28.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/exporters/prometheus/testdata/conflict_help.txt b/exporters/prometheus/testdata/conflict_help.txt new file mode 100644 index 00000000000..103c42bd863 --- /dev/null +++ b/exporters/prometheus/testdata/conflict_help.txt @@ -0,0 +1,11 @@ +# HELP bar_bytes_total meter a bar +# TYPE bar_bytes_total counter +bar_bytes_total{otel_scope_name="ma",otel_scope_version="v0.1.0",type="bar"} 100 +bar_bytes_total{otel_scope_name="mb",otel_scope_version="v0.1.0",type="bar"} 100 +# HELP otel_scope_info Instrumentation Scope metadata +# TYPE otel_scope_info gauge +otel_scope_info{otel_scope_name="ma",otel_scope_version="v0.1.0"} 1 +otel_scope_info{otel_scope_name="mb",otel_scope_version="v0.1.0"} 1 +# HELP target_info Target metadata +# TYPE target_info gauge +target_info{service_name="prometheus_test",telemetry_sdk_language="go",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="latest"} 1 diff --git a/exporters/prometheus/testdata/conflict_type_counter.txt b/exporters/prometheus/testdata/conflict_type_counter.txt new file mode 100644 index 00000000000..c65bebbda0a --- /dev/null +++ b/exporters/prometheus/testdata/conflict_type_counter.txt @@ -0,0 +1,9 @@ +# HELP foz_total meter foz +# TYPE foz_total counter +foz_total{otel_scope_name="ma",otel_scope_version="v0.1.0",type="foz"} 100 +# HELP otel_scope_info Instrumentation Scope metadata +# TYPE otel_scope_info gauge +otel_scope_info{otel_scope_name="ma",otel_scope_version="v0.1.0"} 1 +# HELP target_info Target metadata +# TYPE target_info gauge +target_info{service_name="prometheus_test",telemetry_sdk_language="go",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="latest"} 1 diff --git a/exporters/prometheus/testdata/conflict_unit.txt b/exporters/prometheus/testdata/conflict_unit.txt new file mode 100644 index 00000000000..1902264b98f --- /dev/null +++ b/exporters/prometheus/testdata/conflict_unit.txt @@ -0,0 +1,11 @@ +# HELP baz_total meter baz +# TYPE baz_total counter +baz_total{otel_scope_name="ma",otel_scope_version="v0.1.0",type="baz"} 100 +baz_total{otel_scope_name="mb",otel_scope_version="v0.1.0",type="baz"} 100 +# HELP otel_scope_info Instrumentation Scope metadata +# TYPE otel_scope_info gauge +otel_scope_info{otel_scope_name="ma",otel_scope_version="v0.1.0"} 1 +otel_scope_info{otel_scope_name="mb",otel_scope_version="v0.1.0"} 1 +# HELP target_info Target metadata +# TYPE target_info gauge +target_info{service_name="prometheus_test",telemetry_sdk_language="go",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="latest"} 1 diff --git a/exporters/prometheus/testdata/no_conflict.txt b/exporters/prometheus/testdata/no_conflict.txt new file mode 100644 index 00000000000..dff7fb63600 --- /dev/null +++ b/exporters/prometheus/testdata/no_conflict.txt @@ -0,0 +1,11 @@ +# HELP foo_bytes_total meter foo +# TYPE foo_bytes_total counter +foo_bytes_total{otel_scope_name="ma",otel_scope_version="v0.1.0",type="foo"} 100 +foo_bytes_total{otel_scope_name="mb",otel_scope_version="v0.1.0",type="foo"} 100 +# HELP otel_scope_info Instrumentation Scope metadata +# TYPE otel_scope_info gauge +otel_scope_info{otel_scope_name="ma",otel_scope_version="v0.1.0"} 1 +otel_scope_info{otel_scope_name="mb",otel_scope_version="v0.1.0"} 1 +# HELP target_info Target metadata +# TYPE target_info gauge +target_info{service_name="prometheus_test",telemetry_sdk_language="go",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="latest"} 1