From c7af22a550006fda62863356ec35880be7a6552d Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Wed, 13 Nov 2024 10:01:38 -0800 Subject: [PATCH 1/5] prometheus exporter: validate metric types and help/descriptions --- .chloggen/prometheus-metric-types-help.yaml | 27 +++++ exporter/prometheusexporter/collector.go | 69 ++++++++--- exporter/prometheusexporter/collector_test.go | 108 ++++++++++++++---- 3 files changed, 169 insertions(+), 35 deletions(-) create mode 100644 .chloggen/prometheus-metric-types-help.yaml diff --git a/.chloggen/prometheus-metric-types-help.yaml b/.chloggen/prometheus-metric-types-help.yaml new file mode 100644 index 000000000000..b8167777213a --- /dev/null +++ b/.chloggen/prometheus-metric-types-help.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: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: prometheusexporter + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: reject metrics whose types have changed, use pre-existing descriptions when help strings change + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [28617] + +# (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: [] diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index be676ac7c9a5..4e925a07507d 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), } } @@ -104,7 +108,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) @@ -123,18 +133,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: @@ -162,11 +171,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: @@ -182,7 +196,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 { @@ -218,9 +231,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 { @@ -237,7 +252,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()) @@ -266,7 +284,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 { @@ -404,3 +421,27 @@ 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 description, 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 +} 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) From a5407044a4109cc9cb07f587121a22ae378ff87c Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Wed, 13 Nov 2024 13:32:13 -0800 Subject: [PATCH 2/5] add expiration and cleanup for metric families --- exporter/prometheusexporter/collector.go | 47 ++++++++++++++----- exporter/prometheusexporter/collector_test.go | 40 +++++++++------- 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index 4e925a07507d..9909f7ae901c 100644 --- a/exporter/prometheusexporter/collector.go +++ b/exporter/prometheusexporter/collector.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "fmt" "sort" + "time" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" @@ -32,7 +33,13 @@ type collector struct { addMetricSuffixes bool namespace string constLabels prometheus.Labels - metricFamilies map[string]*dto.MetricFamily + metricFamilies map[string]metricFamily + metricExpiration time.Duration +} + +type metricFamily struct { + lastSeen time.Time + mf *dto.MetricFamily } func newCollector(config *Config, logger *zap.Logger) *collector { @@ -43,7 +50,8 @@ func newCollector(config *Config, logger *zap.Logger) *collector { sendTimestamps: config.SendTimestamps, constLabels: config.ConstLabels, addMetricSuffixes: config.AddMetricSuffixes, - metricFamilies: make(map[string]*dto.MetricFamily), + metricFamilies: make(map[string]metricFamily), + metricExpiration: config.MetricExpiration, } } @@ -420,28 +428,45 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { ch <- m c.logger.Debug(fmt.Sprintf("metric served: %s", m.Desc().String())) } + c.cleanupMetricFamilies() } func (c *collector) validateMetrics(name, description string, metricType *dto.MetricType) (help string, err error) { + now := time.Now() emf, exist := c.metricFamilies[name] if !exist { - c.metricFamilies[name] = &dto.MetricFamily{ - Name: proto.String(name), - Help: proto.String(description), - Type: metricType, + c.metricFamilies[name] = metricFamily{ + lastSeen: now, + mf: &dto.MetricFamily{ + Name: proto.String(name), + Help: proto.String(description), + Type: metricType, + }, } return description, 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.mf.GetType() != *metricType { + return "", fmt.Errorf("instrument type conflict, using existing type definition. instrument: %s, existing: %s, dropped: %s", name, emf.mf.GetType(), *metricType) } - if emf.GetHelp() != description { + emf.lastSeen = now + c.metricFamilies[name] = emf + if emf.mf.GetHelp() != description { c.logger.Info( "Instrument description conflict, using existing", zap.String("instrument", name), - zap.String("existing", emf.GetHelp()), + zap.String("existing", emf.mf.GetHelp()), zap.String("dropped", description), ) } - return emf.GetHelp(), nil + return emf.mf.GetHelp(), nil +} + +func (c *collector) cleanupMetricFamilies() { + expirationTime := time.Now().Add(-c.metricExpiration) + for k, v := range c.metricFamilies { + if expirationTime.After(v.lastSeen) { + c.logger.Debug(fmt.Sprintf("metric expired: %s", k)) + delete(c.metricFamilies, k) + } + } } diff --git a/exporter/prometheusexporter/collector_test.go b/exporter/prometheusexporter/collector_test.go index fe1d68809875..181328db2b98 100644 --- a/exporter/prometheusexporter/collector_test.go +++ b/exporter/prometheusexporter/collector_test.go @@ -50,7 +50,7 @@ func TestConvertInvalidDataType(t *testing.T) { pcommon.NewMap(), }, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } _, err := c.convertMetric(metric, pcommon.NewMap()) @@ -74,35 +74,37 @@ func TestConvertMetric(t *testing.T) { description string mName string mType pmetric.MetricType - mfs map[string]*io_prometheus_client.MetricFamily + mfs map[string]metricFamily err bool }{ { description: "invalid histogram metric", mType: pmetric.MetricTypeHistogram, - mfs: make(map[string]*io_prometheus_client.MetricFamily), + mfs: make(map[string]metricFamily), err: true, }, { description: "invalid sum metric", mType: pmetric.MetricTypeSum, - mfs: make(map[string]*io_prometheus_client.MetricFamily), + mfs: make(map[string]metricFamily), err: true, }, { description: "invalid gauge metric", mType: pmetric.MetricTypeGauge, - mfs: make(map[string]*io_prometheus_client.MetricFamily), + mfs: make(map[string]metricFamily), err: true, }, { description: "metric type conflict", mName: "testgauge", mType: pmetric.MetricTypeGauge, - mfs: map[string]*io_prometheus_client.MetricFamily{ + mfs: map[string]metricFamily{ "testgauge": { - Name: proto.String("testgauge"), - Type: dto.MetricType_COUNTER.Enum(), + mf: &io_prometheus_client.MetricFamily{ + Name: proto.String("testgauge"), + Type: dto.MetricType_COUNTER.Enum(), + }, }, }, err: true, @@ -111,11 +113,13 @@ func TestConvertMetric(t *testing.T) { description: "metric description conflict", mName: "testgauge", mType: pmetric.MetricTypeGauge, - mfs: map[string]*io_prometheus_client.MetricFamily{ + mfs: map[string]metricFamily{ "testgauge": { - Name: proto.String("testgauge"), - Type: dto.MetricType_GAUGE.Enum(), - Help: proto.String("test help value"), + mf: &io_prometheus_client.MetricFamily{ + Name: proto.String("testgauge"), + Type: dto.MetricType_GAUGE.Enum(), + Help: proto.String("test help value"), + }, }, }, err: false, @@ -224,7 +228,7 @@ func TestConvertDoubleHistogramExemplar(t *testing.T) { resourceAttributes: pMap, }, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } pbMetric, _ := c.convertDoubleHistogram(metric, pMap) @@ -267,7 +271,7 @@ func TestConvertMonotonicSumExemplar(t *testing.T) { resourceAttributes: pMap, }, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } promMetric, _ := c.convertSum(metric, pMap) @@ -322,7 +326,7 @@ func TestCollectMetricsLabelSanitize(t *testing.T) { }, sendTimestamps: false, logger: zap.New(&loggerCore), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -531,7 +535,7 @@ func TestCollectMetrics(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -655,7 +659,7 @@ func TestAccumulateHistograms(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -766,7 +770,7 @@ func TestAccumulateSummary(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]*io_prometheus_client.MetricFamily), + metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) From 5a8159c76729305f87cb35566e9500250ba372f6 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Thu, 14 Nov 2024 10:23:10 -0800 Subject: [PATCH 3/5] add sync.map --- exporter/prometheusexporter/collector.go | 25 ++++++++++------- exporter/prometheusexporter/collector_test.go | 28 +++++++------------ 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index 9909f7ae901c..2e2ff1e96e0e 100644 --- a/exporter/prometheusexporter/collector.go +++ b/exporter/prometheusexporter/collector.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "fmt" "sort" + "sync" "time" "github.com/prometheus/client_golang/prometheus" @@ -33,7 +34,7 @@ type collector struct { addMetricSuffixes bool namespace string constLabels prometheus.Labels - metricFamilies map[string]metricFamily + metricFamilies sync.Map metricExpiration time.Duration } @@ -50,7 +51,6 @@ func newCollector(config *Config, logger *zap.Logger) *collector { sendTimestamps: config.SendTimestamps, constLabels: config.ConstLabels, addMetricSuffixes: config.AddMetricSuffixes, - metricFamilies: make(map[string]metricFamily), metricExpiration: config.MetricExpiration, } } @@ -433,23 +433,24 @@ func (c *collector) Collect(ch chan<- prometheus.Metric) { func (c *collector) validateMetrics(name, description string, metricType *dto.MetricType) (help string, err error) { now := time.Now() - emf, exist := c.metricFamilies[name] + v, exist := c.metricFamilies.Load(name) if !exist { - c.metricFamilies[name] = metricFamily{ + c.metricFamilies.Store(name, metricFamily{ lastSeen: now, mf: &dto.MetricFamily{ Name: proto.String(name), Help: proto.String(description), Type: metricType, }, - } + }) return description, nil } + emf := v.(metricFamily) if emf.mf.GetType() != *metricType { return "", fmt.Errorf("instrument type conflict, using existing type definition. instrument: %s, existing: %s, dropped: %s", name, emf.mf.GetType(), *metricType) } emf.lastSeen = now - c.metricFamilies[name] = emf + c.metricFamilies.Store(name, emf) if emf.mf.GetHelp() != description { c.logger.Info( "Instrument description conflict, using existing", @@ -463,10 +464,14 @@ func (c *collector) validateMetrics(name, description string, metricType *dto.Me func (c *collector) cleanupMetricFamilies() { expirationTime := time.Now().Add(-c.metricExpiration) - for k, v := range c.metricFamilies { + + c.metricFamilies.Range(func(key, value any) bool { + v := value.(metricFamily) if expirationTime.After(v.lastSeen) { - c.logger.Debug(fmt.Sprintf("metric expired: %s", k)) - delete(c.metricFamilies, k) + c.logger.Debug(fmt.Sprintf("metric expired: %s", key)) + c.metricFamilies.Delete(key) + return true } - } + return true + }) } diff --git a/exporter/prometheusexporter/collector_test.go b/exporter/prometheusexporter/collector_test.go index 181328db2b98..f1b87d9fe10b 100644 --- a/exporter/prometheusexporter/collector_test.go +++ b/exporter/prometheusexporter/collector_test.go @@ -49,8 +49,7 @@ func TestConvertInvalidDataType(t *testing.T) { []pmetric.Metric{metric}, pcommon.NewMap(), }, - logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), + logger: zap.NewNop(), } _, err := c.convertMetric(metric, pcommon.NewMap()) @@ -74,32 +73,29 @@ func TestConvertMetric(t *testing.T) { description string mName string mType pmetric.MetricType - mfs map[string]metricFamily + mapVals map[string]metricFamily err bool }{ { description: "invalid histogram metric", mType: pmetric.MetricTypeHistogram, - mfs: make(map[string]metricFamily), err: true, }, { description: "invalid sum metric", mType: pmetric.MetricTypeSum, - mfs: make(map[string]metricFamily), err: true, }, { description: "invalid gauge metric", mType: pmetric.MetricTypeGauge, - mfs: make(map[string]metricFamily), err: true, }, { description: "metric type conflict", mName: "testgauge", mType: pmetric.MetricTypeGauge, - mfs: map[string]metricFamily{ + mapVals: map[string]metricFamily{ "testgauge": { mf: &io_prometheus_client.MetricFamily{ Name: proto.String("testgauge"), @@ -113,7 +109,7 @@ func TestConvertMetric(t *testing.T) { description: "metric description conflict", mName: "testgauge", mType: pmetric.MetricTypeGauge, - mfs: map[string]metricFamily{ + mapVals: map[string]metricFamily{ "testgauge": { mf: &io_prometheus_client.MetricFamily{ Name: proto.String("testgauge"), @@ -138,8 +134,10 @@ func TestConvertMetric(t *testing.T) { metric.SetEmptyHistogram().DataPoints().AppendEmpty() } c := collector{ - logger: zap.NewNop(), - metricFamilies: tt.mfs, + logger: zap.NewNop(), + } + for k, v := range tt.mapVals { + c.metricFamilies.Store(k, v) } _, err := c.convertMetric(metric, pcommon.NewMap()) @@ -227,8 +225,7 @@ func TestConvertDoubleHistogramExemplar(t *testing.T) { metrics: []pmetric.Metric{metric}, resourceAttributes: pMap, }, - logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), + logger: zap.NewNop(), } pbMetric, _ := c.convertDoubleHistogram(metric, pMap) @@ -270,8 +267,7 @@ func TestConvertMonotonicSumExemplar(t *testing.T) { metrics: []pmetric.Metric{metric}, resourceAttributes: pMap, }, - logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), + logger: zap.NewNop(), } promMetric, _ := c.convertSum(metric, pMap) @@ -326,7 +322,6 @@ func TestCollectMetricsLabelSanitize(t *testing.T) { }, sendTimestamps: false, logger: zap.New(&loggerCore), - metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -535,7 +530,6 @@ func TestCollectMetrics(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -659,7 +653,6 @@ func TestAccumulateHistograms(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) @@ -770,7 +763,6 @@ func TestAccumulateSummary(t *testing.T) { }, sendTimestamps: sendTimestamp, logger: zap.NewNop(), - metricFamilies: make(map[string]metricFamily), } ch := make(chan prometheus.Metric, 1) From b4f49065be35750e47b047101a30e140d1748c0c Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 18 Nov 2024 09:35:28 -0800 Subject: [PATCH 4/5] fix debug log --- exporter/prometheusexporter/collector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index 2e2ff1e96e0e..4067b1e1aa73 100644 --- a/exporter/prometheusexporter/collector.go +++ b/exporter/prometheusexporter/collector.go @@ -468,7 +468,7 @@ func (c *collector) cleanupMetricFamilies() { c.metricFamilies.Range(func(key, value any) bool { v := value.(metricFamily) if expirationTime.After(v.lastSeen) { - c.logger.Debug(fmt.Sprintf("metric expired: %s", key)) + c.logger.Debug(fmt.Sprintf("metric expired"), zap.String("instrument", key.(string))) c.metricFamilies.Delete(key) return true } From 3786684c777f9ea3cf220ef887f150d786803793 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 18 Nov 2024 09:47:39 -0800 Subject: [PATCH 5/5] Update exporter/prometheusexporter/collector.go Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> --- exporter/prometheusexporter/collector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporter/prometheusexporter/collector.go b/exporter/prometheusexporter/collector.go index 4067b1e1aa73..42035add2fbc 100644 --- a/exporter/prometheusexporter/collector.go +++ b/exporter/prometheusexporter/collector.go @@ -468,7 +468,7 @@ func (c *collector) cleanupMetricFamilies() { c.metricFamilies.Range(func(key, value any) bool { v := value.(metricFamily) if expirationTime.After(v.lastSeen) { - c.logger.Debug(fmt.Sprintf("metric expired"), zap.String("instrument", key.(string))) + c.logger.Debug("metric expired", zap.String("instrument", key.(string))) c.metricFamilies.Delete(key) return true }