From 9b544258c340d75c9878fc1067b0b3c9e29d717d Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Wed, 13 Nov 2024 10:01:38 -0800 Subject: [PATCH] prometheus exporter: validate metric types and help/descriptions --- exporter/prometheusexporter/collector.go | 70 +++++++++--- exporter/prometheusexporter/collector_test.go | 108 ++++++++++++++---- 2 files changed, 143 insertions(+), 35 deletions(-) diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index f6065307eb34..d6796df932df 100644 --- a/exporter/prometheusexporter/collector.go +++ b/exporter/prometheusexporter/collector.go @@ -9,11 +9,13 @@ import ( "sort" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" "github.com/prometheus/common/model" "go.opentelemetry.io/collector/pdata/pcommon" "go.opentelemetry.io/collector/pdata/pmetric" conventions "go.opentelemetry.io/collector/semconv/v1.25.0" "go.uber.org/zap" + "google.golang.org/protobuf/proto" prometheustranslator "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus" ) @@ -30,6 +32,7 @@ type collector struct { addMetricSuffixes bool namespace string constLabels prometheus.Labels + metricFamilies map[string]*dto.MetricFamily } func newCollector(config *Config, logger *zap.Logger) *collector { @@ -40,6 +43,7 @@ func newCollector(config *Config, logger *zap.Logger) *collector { sendTimestamps: config.SendTimestamps, constLabels: config.ConstLabels, addMetricSuffixes: config.AddMetricSuffixes, + metricFamilies: make(map[string]*dto.MetricFamily), } } @@ -105,7 +109,13 @@ func (c *collector) convertMetric(metric pmetric.Metric, resourceAttrs pcommon.M return nil, errUnknownMetricType } -func (c *collector) getMetricMetadata(metric pmetric.Metric, attributes pcommon.Map, resourceAttrs pcommon.Map) (*prometheus.Desc, []string) { +func (c *collector) getMetricMetadata(metric pmetric.Metric, mType *dto.MetricType, attributes pcommon.Map, resourceAttrs pcommon.Map) (*prometheus.Desc, []string, error) { + name := prometheustranslator.BuildCompliantName(metric, c.namespace, c.addMetricSuffixes) + help, err := c.validateMetrics(name, metric.Description(), mType) + if err != nil { + return nil, nil, err + } + keys := make([]string, 0, attributes.Len()+2) // +2 for job and instance labels. values := make([]string, 0, attributes.Len()+2) @@ -124,18 +134,17 @@ func (c *collector) getMetricMetadata(metric pmetric.Metric, attributes pcommon. values = append(values, instance) } - return prometheus.NewDesc( - prometheustranslator.BuildCompliantName(metric, c.namespace, c.addMetricSuffixes), - metric.Description(), - keys, - c.constLabels, - ), values + return prometheus.NewDesc(name, help, keys, c.constLabels), values, nil } func (c *collector) convertGauge(metric pmetric.Metric, resourceAttrs pcommon.Map) (prometheus.Metric, error) { ip := metric.Gauge().DataPoints().At(0) - desc, attributes := c.getMetricMetadata(metric, ip.Attributes(), resourceAttrs) + desc, attributes, err := c.getMetricMetadata(metric, dto.MetricType_GAUGE.Enum(), ip.Attributes(), resourceAttrs) + if err != nil { + return nil, err + } + var value float64 switch ip.ValueType() { case pmetric.NumberDataPointValueTypeInt: @@ -163,11 +172,16 @@ func (c *collector) convertSum(metric pmetric.Metric, resourceAttrs pcommon.Map) ip := metric.Sum().DataPoints().At(0) metricType := prometheus.GaugeValue + mType := dto.MetricType_GAUGE.Enum() if metric.Sum().IsMonotonic() { metricType = prometheus.CounterValue + mType = dto.MetricType_COUNTER.Enum() } - desc, attributes := c.getMetricMetadata(metric, ip.Attributes(), resourceAttrs) + desc, attributes, err := c.getMetricMetadata(metric, mType, ip.Attributes(), resourceAttrs) + if err != nil { + return nil, err + } var value float64 switch ip.ValueType() { case pmetric.NumberDataPointValueTypeInt: @@ -183,7 +197,6 @@ func (c *collector) convertSum(metric pmetric.Metric, resourceAttrs pcommon.Map) } var m prometheus.Metric - var err error if metricType == prometheus.CounterValue && ip.StartTimestamp().AsTime().Unix() > 0 { m, err = prometheus.NewConstMetricWithCreatedTimestamp(desc, metricType, value, ip.StartTimestamp().AsTime(), attributes...) } else { @@ -219,9 +232,11 @@ func (c *collector) convertSummary(metric pmetric.Metric, resourceAttrs pcommon. quantiles[qvj.Quantile()] = qvj.Value() } - desc, attributes := c.getMetricMetadata(metric, point.Attributes(), resourceAttrs) + desc, attributes, err := c.getMetricMetadata(metric, dto.MetricType_SUMMARY.Enum(), point.Attributes(), resourceAttrs) + if err != nil { + return nil, err + } var m prometheus.Metric - var err error if point.StartTimestamp().AsTime().Unix() > 0 { m, err = prometheus.NewConstSummaryWithCreatedTimestamp(desc, point.Count(), point.Sum(), quantiles, point.StartTimestamp().AsTime(), attributes...) } else { @@ -238,7 +253,10 @@ func (c *collector) convertSummary(metric pmetric.Metric, resourceAttrs pcommon. func (c *collector) convertDoubleHistogram(metric pmetric.Metric, resourceAttrs pcommon.Map) (prometheus.Metric, error) { ip := metric.Histogram().DataPoints().At(0) - desc, attributes := c.getMetricMetadata(metric, ip.Attributes(), resourceAttrs) + desc, attributes, err := c.getMetricMetadata(metric, dto.MetricType_HISTOGRAM.Enum(), ip.Attributes(), resourceAttrs) + if err != nil { + return nil, err + } indicesMap := make(map[float64]int) buckets := make([]float64, 0, ip.BucketCounts().Len()) @@ -267,7 +285,6 @@ func (c *collector) convertDoubleHistogram(metric pmetric.Metric, resourceAttrs exemplars := convertExemplars(ip.Exemplars()) var m prometheus.Metric - var err error if ip.StartTimestamp().AsTime().Unix() > 0 { m, err = prometheus.NewConstHistogramWithCreatedTimestamp(desc, ip.Count(), ip.Sum(), points, ip.StartTimestamp().AsTime(), attributes...) } else { @@ -405,3 +422,28 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { c.logger.Debug(fmt.Sprintf("metric served: %s", m.Desc().String())) } } + +func (c *collector) validateMetrics(name, description string, metricType *dto.MetricType) (help string, err error) { + emf, exist := c.metricFamilies[name] + if !exist { + c.metricFamilies[name] = &dto.MetricFamily{ + Name: proto.String(name), + Help: proto.String(description), + Type: metricType, + } + return "", nil + } + if emf.GetType() != *metricType { + return "", fmt.Errorf("instrument type conflict, using existing type definition. instrument: %s, existing: %s, dropped: %s", name, emf.GetType(), *metricType) + } + if emf.GetHelp() != description { + c.logger.Info( + "Instrument description conflict, using existing", + zap.String("instrument", name), + zap.String("existing", emf.GetHelp()), + zap.String("dropped", description), + ) + return emf.GetHelp(), nil + } + return "", nil +} diff --git a/exporter/prometheusexporter/collector_test.go b/exporter/prometheusexporter/collector_test.go index 9b5d31d7efdb..fe1d68809875 100644 --- a/exporter/prometheusexporter/collector_test.go +++ b/exporter/prometheusexporter/collector_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" io_prometheus_client "github.com/prometheus/client_model/go" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/pdata/pcommon" @@ -17,6 +18,7 @@ import ( conventions "go.opentelemetry.io/collector/semconv/v1.25.0" "go.uber.org/zap" "go.uber.org/zap/zapcore" + "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" prometheustranslator "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus" @@ -47,7 +49,8 @@ func TestConvertInvalidDataType(t *testing.T) { []pmetric.Metric{metric}, pcommon.NewMap(), }, - logger: zap.NewNop(), + logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } _, err := c.convertMetric(metric, pcommon.NewMap()) @@ -66,25 +69,82 @@ func TestConvertInvalidDataType(t *testing.T) { } } -func TestConvertInvalidMetric(t *testing.T) { - for _, mType := range []pmetric.MetricType{ - pmetric.MetricTypeHistogram, - pmetric.MetricTypeSum, - pmetric.MetricTypeGauge, - } { - metric := pmetric.NewMetric() - switch mType { - case pmetric.MetricTypeGauge: - metric.SetEmptyGauge().DataPoints().AppendEmpty() - case pmetric.MetricTypeSum: - metric.SetEmptySum().DataPoints().AppendEmpty() - case pmetric.MetricTypeHistogram: - metric.SetEmptyHistogram().DataPoints().AppendEmpty() - } - c := collector{} +func TestConvertMetric(t *testing.T) { + tests := []struct { + description string + mName string + mType pmetric.MetricType + mfs map[string]*io_prometheus_client.MetricFamily + err bool + }{ + { + description: "invalid histogram metric", + mType: pmetric.MetricTypeHistogram, + mfs: make(map[string]*io_prometheus_client.MetricFamily), + err: true, + }, + { + description: "invalid sum metric", + mType: pmetric.MetricTypeSum, + mfs: make(map[string]*io_prometheus_client.MetricFamily), + err: true, + }, + { + description: "invalid gauge metric", + mType: pmetric.MetricTypeGauge, + mfs: make(map[string]*io_prometheus_client.MetricFamily), + err: true, + }, + { + description: "metric type conflict", + mName: "testgauge", + mType: pmetric.MetricTypeGauge, + mfs: map[string]*io_prometheus_client.MetricFamily{ + "testgauge": { + Name: proto.String("testgauge"), + Type: dto.MetricType_COUNTER.Enum(), + }, + }, + err: true, + }, + { + description: "metric description conflict", + mName: "testgauge", + mType: pmetric.MetricTypeGauge, + mfs: map[string]*io_prometheus_client.MetricFamily{ + "testgauge": { + Name: proto.String("testgauge"), + Type: dto.MetricType_GAUGE.Enum(), + Help: proto.String("test help value"), + }, + }, + err: false, + }, + } + for _, tt := range tests { + t.Run(tt.description, func(t *testing.T) { + metric := pmetric.NewMetric() + metric.SetName(tt.mName) + switch tt.mType { + case pmetric.MetricTypeGauge: + metric.SetEmptyGauge().DataPoints().AppendEmpty() + case pmetric.MetricTypeSum: + metric.SetEmptySum().DataPoints().AppendEmpty() + case pmetric.MetricTypeHistogram: + metric.SetEmptyHistogram().DataPoints().AppendEmpty() + } + c := collector{ + logger: zap.NewNop(), + metricFamilies: tt.mfs, + } - _, err := c.convertMetric(metric, pcommon.NewMap()) - require.Error(t, err) + _, err := c.convertMetric(metric, pcommon.NewMap()) + if tt.err { + require.Error(t, err) + return + } + require.NoError(t, err) + }) } } @@ -163,7 +223,8 @@ func TestConvertDoubleHistogramExemplar(t *testing.T) { metrics: []pmetric.Metric{metric}, resourceAttributes: pMap, }, - logger: zap.NewNop(), + logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } pbMetric, _ := c.convertDoubleHistogram(metric, pMap) @@ -205,7 +266,8 @@ func TestConvertMonotonicSumExemplar(t *testing.T) { metrics: []pmetric.Metric{metric}, resourceAttributes: pMap, }, - logger: zap.NewNop(), + logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } promMetric, _ := c.convertSum(metric, pMap) @@ -260,6 +322,7 @@ func TestCollectMetricsLabelSanitize(t *testing.T) { }, sendTimestamps: false, logger: zap.New(&loggerCore), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } ch := make(chan prometheus.Metric, 1) @@ -468,6 +531,7 @@ func TestCollectMetrics(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } ch := make(chan prometheus.Metric, 1) @@ -591,6 +655,7 @@ func TestAccumulateHistograms(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } ch := make(chan prometheus.Metric, 1) @@ -701,6 +766,7 @@ func TestAccumulateSummary(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), + metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), } ch := make(chan prometheus.Metric, 1)