From 40ed2839507f1d4edabbf1f92277c277e1012dee Mon Sep 17 00:00:00 2001 From: Ziqi Zhao Date: Sun, 20 Nov 2022 18:57:51 +0800 Subject: [PATCH] more logic Signed-off-by: Ziqi Zhao --- exporters/prometheus/exporter.go | 101 +++++++++++++----- exporters/prometheus/exporter_test.go | 18 ++++ .../duplicate_metrics_conflict_type.txt | 9 ++ sdk/metric/metricdata/data.go | 10 -- 4 files changed, 100 insertions(+), 38 deletions(-) create mode 100644 exporters/prometheus/testdata/duplicate_metrics_conflict_type.txt diff --git a/exporters/prometheus/exporter.go b/exporters/prometheus/exporter.go index 2a67b92ed3d..8341d2e3054 100644 --- a/exporters/prometheus/exporter.go +++ b/exporters/prometheus/exporter.go @@ -23,7 +23,9 @@ import ( "unicode" "unicode/utf8" + "github.com/golang/protobuf/proto" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -62,12 +64,7 @@ type collector struct { disableScopeInfo bool createTargetInfoOnce sync.Once scopeInfos map[instrumentation.Scope]prometheus.Metric - metricInfos map[string]metricdata.MetricInfo -} - -type metricInfo struct { - description string - unit unit.Unit + metricFamiliesByName map[string]*dto.MetricFamily } // prometheus counters MUST have a _total suffix: @@ -84,12 +81,12 @@ func New(opts ...Option) (*Exporter, error) { reader := metric.NewManualReader(cfg.manualReaderOptions()...) collector := &collector{ - reader: reader, - disableTargetInfo: cfg.disableTargetInfo, - withoutUnits: cfg.withoutUnits, - disableScopeInfo: cfg.disableScopeInfo, - scopeInfos: make(map[instrumentation.Scope]prometheus.Metric), - metricInfos: make(map[string]metricdata.MetricInfo), + reader: reader, + disableTargetInfo: cfg.disableTargetInfo, + withoutUnits: cfg.withoutUnits, + disableScopeInfo: cfg.disableScopeInfo, + scopeInfos: make(map[instrumentation.Scope]prometheus.Metric), + metricFamiliesByName: make(map[string]*dto.MetricFamily), } if err := cfg.registerer.Register(collector); err != nil { @@ -154,8 +151,11 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { } for _, m := range scopeMetrics.Metrics { - validated, m := c.validateMetricInfo(m) - if !validated { + drop, m, err := c.validateMetrics(m) + if err != nil { + fmt.Printf("warning: %v\n", err) + } + if drop { continue } @@ -357,7 +357,8 @@ func sanitizeName(n string) string { return b.String() } -func (c *collector) validateMetricInfo(m metricdata.Metrics) (bool, metricdata.Metrics) { +func (c *collector) validateMetrics(m metricdata.Metrics) (bool, metricdata.Metrics, error) { + // get the name of metric name name := c.getName(m) switch m.Data.(type) { case metricdata.Sum[int64]: @@ -370,26 +371,70 @@ func (c *collector) validateMetricInfo(m metricdata.Metrics) (bool, metricdata.M if sum.IsMonotonic { name += counterSuffix } - default: } - metric, exist := c.metricInfos[name] - if !exist { - c.metricInfos[name] = metricdata.MetricInfo{ - Name: name, - Description: m.Description, - Unit: m.Unit, + metricFamily := &dto.MetricFamily{ + Name: proto.String(name), + Help: proto.String(m.Description), + } + switch m.Data.(type) { + case metricdata.Sum[int64]: + sum := m.Data.(metricdata.Sum[int64]) + metricFamily.Type = dto.MetricType_COUNTER.Enum() + if !sum.IsMonotonic { + metricFamily.Type = dto.MetricType_GAUGE.Enum() } - return true, m + case metricdata.Sum[float64]: + sum := m.Data.(metricdata.Sum[float64]) + metricFamily.Type = dto.MetricType_COUNTER.Enum() + if !sum.IsMonotonic { + metricFamily.Type = dto.MetricType_GAUGE.Enum() + } + case metricdata.Gauge[int64]: + metricFamily.Type = dto.MetricType_GAUGE.Enum() + case metricdata.Gauge[float64]: + metricFamily.Type = dto.MetricType_GAUGE.Enum() + case metricdata.Histogram: + metricFamily.Type = dto.MetricType_HISTOGRAM.Enum() + } + + collision := validateCollision(metricFamily, c.metricFamiliesByName) + switch collision { + case noCollision: + c.metricFamiliesByName[name] = metricFamily + return false, m, nil + case typeCollision: + return true, m, fmt.Errorf("type collision") + case helpCollision: + emf := c.metricFamiliesByName[name] + m.Description = emf.GetHelp() + return false, m, fmt.Errorf("help collision") + } + + return false, m, nil +} + +type collisionType int + +const ( + noCollision collisionType = 0 + typeCollision collisionType = 1 + helpCollision collisionType = 2 +) + +func validateCollision(mf *dto.MetricFamily, mfs map[string]*dto.MetricFamily) collisionType { + emf, exist := mfs[mf.GetName()] + if !exist { + return noCollision } - if metric.Description != m.Description { - m.Description = metric.Description + if emf.GetType() != mf.GetType() { + return typeCollision } - if metric.Unit != m.Unit { - m.Unit = metric.Unit + if emf.GetHelp() != mf.GetHelp() { + return helpCollision } - return true, m + return noCollision } diff --git a/exporters/prometheus/exporter_test.go b/exporters/prometheus/exporter_test.go index 3a82f81687b..3b23dcafea9 100644 --- a/exporters/prometheus/exporter_test.go +++ b/exporters/prometheus/exporter_test.go @@ -454,6 +454,24 @@ func TestDuplicateMetrics(t *testing.T) { options: []Option{WithoutUnits()}, expectedFile: "testdata/duplicate_metrics_conflict_unit.txt", }, + { + name: "conflict_type", + recordMetrics: func(ctx context.Context, meterA, meterB otelmetric.Meter) { + counter, err := meterA.SyncInt64().Counter("foo", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foo")) + assert.NoError(t, err) + counter.Add(ctx, 100, attribute.String("type", "foo")) + + gauge, err := meterA.SyncInt64().UpDownCounter("foo_total", + instrument.WithUnit(unit.Bytes), + instrument.WithDescription("meter foo")) + assert.NoError(t, err) + gauge.Add(ctx, 200, attribute.String("type", "foo")) + }, + options: []Option{WithoutUnits()}, + expectedFile: "testdata/duplicate_metrics_conflict_type.txt", + }, } for _, tc := range testCases { diff --git a/exporters/prometheus/testdata/duplicate_metrics_conflict_type.txt b/exporters/prometheus/testdata/duplicate_metrics_conflict_type.txt new file mode 100644 index 00000000000..885fb3c7259 --- /dev/null +++ b/exporters/prometheus/testdata/duplicate_metrics_conflict_type.txt @@ -0,0 +1,9 @@ +# HELP foo_total meter foo +# TYPE foo_total counter +foo_total{otel_scope_name="ma",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 +# 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/sdk/metric/metricdata/data.go b/sdk/metric/metricdata/data.go index 6c81c828e4b..a729879e673 100644 --- a/sdk/metric/metricdata/data.go +++ b/sdk/metric/metricdata/data.go @@ -52,16 +52,6 @@ type Metrics struct { Data Aggregation } -// MetricInfo is basic info of an Instrument -type MetricInfo struct { - // Name is the name of the Instrument that created this data. - Name string - // Description is the description of the Instrument, which can be used in documentation. - Description string - // Unit is the unit in which the Instrument reports. - Unit unit.Unit -} - // Aggregation is the store of data reported by an Instrument. // It will be one of: Gauge, Sum, Histogram. type Aggregation interface {