From c10e95e1d21c013956e77cb82b0e40123f34a439 Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Tue, 3 May 2022 14:18:42 +0000 Subject: [PATCH] return errors when naming unsupported types --- exporter/collector/config.go | 2 +- .../googlemanagedprometheus/naming.go | 17 ++-- .../googlemanagedprometheus/naming_test.go | 60 +++++++++--- exporter/collector/metrics.go | 94 ++++++++++++++----- exporter/collector/metrics_test.go | 8 +- 5 files changed, 132 insertions(+), 49 deletions(-) diff --git a/exporter/collector/config.go b/exporter/collector/config.go index 63326162c..d358c87d8 100644 --- a/exporter/collector/config.go +++ b/exporter/collector/config.go @@ -113,7 +113,7 @@ type MetricConfig struct { // exporters which extend the functionality of this exporter. It allows // customizing the naming of metrics. baseName already includes type // suffixes for summary metrics, but does not (yet) include the domain prefix - GetMetricName func(baseName string, metric pdata.Metric) string + GetMetricName func(baseName string, metric pdata.Metric) (string, error) // MapMonitoredResource is not exposed as an option in the configuration, but // can be used by other exporters to extend the functionality of this diff --git a/exporter/collector/googlemanagedprometheus/naming.go b/exporter/collector/googlemanagedprometheus/naming.go index e739aa195..f40046680 100644 --- a/exporter/collector/googlemanagedprometheus/naming.go +++ b/exporter/collector/googlemanagedprometheus/naming.go @@ -15,32 +15,31 @@ package googlemanagedprometheus import ( + "fmt" "strings" "go.opentelemetry.io/collector/model/pdata" "go.opentelemetry.io/collector/pdata/pmetric" ) -func GetMetricName(baseName string, metric pdata.Metric) string { +func GetMetricName(baseName string, metric pdata.Metric) (string, error) { switch metric.DataType() { case pmetric.MetricDataTypeSum: - return baseName + "/counter" + return baseName + "/counter", nil case pmetric.MetricDataTypeGauge: - return baseName + "/gauge" + return baseName + "/gauge", nil case pmetric.MetricDataTypeSummary: // summaries are sent as the following series: // * Sum: prometheus.googleapis.com/_sum/summary:counter // * Count: prometheus.googleapis.com/_count/summary // * Quantiles: prometheus.googleapis.com//summary if strings.HasSuffix(baseName, "_sum") { - return baseName + "/summary:counter" + return baseName + "/summary:counter", nil } - return baseName + "/summary" + return baseName + "/summary", nil case pmetric.MetricDataTypeHistogram: - return baseName + "/histogram" + return baseName + "/histogram", nil default: - // This should never happen, as we have already dropped other data - // types at this point. - return "" + return "", fmt.Errorf("unsupported metric datatype: %v", metric.DataType()) } } diff --git a/exporter/collector/googlemanagedprometheus/naming_test.go b/exporter/collector/googlemanagedprometheus/naming_test.go index 5b13cd559..055942d07 100644 --- a/exporter/collector/googlemanagedprometheus/naming_test.go +++ b/exporter/collector/googlemanagedprometheus/naming_test.go @@ -24,40 +24,74 @@ import ( func TestGetMetricName(t *testing.T) { baseName := "my_metric_name" for _, tc := range []struct { - desc string - datatype pmetric.MetricDataType - expected string + desc string + baseName string + datatype pmetric.MetricDataType + expected string + expectErr bool }{ { desc: "sum", + baseName: "foo_total", datatype: pmetric.MetricDataTypeSum, - expected: baseName + "/counter", + expected: "foo_total/counter", }, { desc: "gauge", + baseName: "bar", datatype: pmetric.MetricDataTypeGauge, - expected: baseName + "/gauge", + expected: "bar/gauge", }, { - desc: "summary", + desc: "summary sum", + baseName: "baz_sum", datatype: pmetric.MetricDataTypeSummary, - expected: baseName + "/summary", + expected: "baz_sum/summary:counter", }, { - desc: "histogram", + desc: "summary count", + baseName: "baz_count", + datatype: pmetric.MetricDataTypeSummary, + expected: "baz_count/summary", + }, + { + desc: "summary quantile", + baseName: "baz", + datatype: pmetric.MetricDataTypeSummary, + expected: "baz/summary", + }, + { + desc: "histogram sum", + baseName: "hello_sum", datatype: pmetric.MetricDataTypeHistogram, - expected: baseName + "/histogram", + expected: "hello_sum/histogram", }, { - desc: "other", - datatype: pmetric.MetricDataTypeExponentialHistogram, - expected: baseName + "/exponentialhistogram", + desc: "histogram count", + baseName: "hello_count", + datatype: pmetric.MetricDataTypeHistogram, + expected: "hello_count/histogram", + }, + { + desc: "histogram bucket", + baseName: "hello", + datatype: pmetric.MetricDataTypeHistogram, + expected: "hello/histogram", + }, + { + desc: "other", + baseName: "other", + datatype: pmetric.MetricDataTypeExponentialHistogram, + expectErr: true, }, } { t.Run(tc.desc, func(t *testing.T) { metric := pdata.NewMetric() metric.SetDataType(tc.datatype) - got := GetMetricName(baseName, metric) + got, err := GetMetricName(tc.baseName, metric) + if tc.expectErr == (err == nil) { + t.Errorf("MetricName(%v, %v)=err(%v); want err: %v", baseName, tc.datatype, err, tc.expectErr) + } if got != tc.expected { t.Errorf("MetricName(%v, %v)=%v; want %v", baseName, tc.datatype, got, tc.expected) } diff --git a/exporter/collector/metrics.go b/exporter/collector/metrics.go index e9d62a74b..a3ea7e8be 100644 --- a/exporter/collector/metrics.go +++ b/exporter/collector/metrics.go @@ -394,7 +394,11 @@ func (m *metricMapper) summaryPointToTimeSeries( return nil } point = *normalizedPoint - sumName, countName := summaryMetricNames(metric.Name()) + sumName, countName, quantileName, err := m.summaryMetricNames(metric) + if err != nil { + m.obs.log.Debug("Failed to get metric name for summary metric. Dropping the metric.", zap.Error(err), zap.Any("metric", metric)) + return nil + } startTime := timestamppb.New(point.StartTimestamp().AsTime()) endTime := timestamppb.New(point.Timestamp().AsTime()) result := []*monitoringpb.TimeSeries{ @@ -413,7 +417,7 @@ func (m *metricMapper) summaryPointToTimeSeries( }}, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(sumName, metric), + Type: sumName, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -435,7 +439,7 @@ func (m *metricMapper) summaryPointToTimeSeries( }}, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(countName, metric), + Type: countName, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -463,7 +467,7 @@ func (m *metricMapper) summaryPointToTimeSeries( }}, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(metric.Name(), metric), + Type: quantileName, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -634,6 +638,11 @@ func (m *metricMapper) histogramToTimeSeries( // Drop points without a value or without a sum return nil } + name, err := m.metricNameToType(metric.Name(), metric) + if err != nil { + m.obs.log.Debug("Failed to get metric name for histogram metric. Dropping the metric.", zap.Error(err), zap.Any("metric", metric)) + return nil + } if hist.AggregationTemporality() == pmetric.MetricAggregationTemporalityCumulative { // Normalize cumulative histogram points. metricIdentifier := datapointstorage.Identifier(resource, extraLabels, metric, point.Attributes()) @@ -662,7 +671,7 @@ func (m *metricMapper) histogramToTimeSeries( Value: value, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(metric.Name(), metric), + Type: name, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -682,6 +691,11 @@ func (m *metricMapper) exponentialHistogramToTimeSeries( // Drop points without a value. return nil } + name, err := m.metricNameToType(metric.Name(), metric) + if err != nil { + m.obs.log.Debug("Failed to get metric name for exponential histogram metric. Dropping the metric.", zap.Error(err), zap.Any("metric", metric)) + return nil + } if exponentialHist.AggregationTemporality() == pmetric.MetricAggregationTemporalityCumulative { // Normalize the histogram point. metricIdentifier := datapointstorage.Identifier(resource, extraLabels, metric, point.Attributes()) @@ -709,7 +723,7 @@ func (m *metricMapper) exponentialHistogramToTimeSeries( Value: value, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(metric.Name(), metric), + Type: name, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -732,6 +746,11 @@ func (m *metricMapper) sumPointToTimeSeries( // prometheus. return nil } + name, err := m.metricNameToType(metric.Name(), metric) + if err != nil { + m.obs.log.Debug("Failed to get metric name for sum metric. Dropping the metric.", zap.Error(err), zap.Any("metric", metric)) + return nil + } if sum.IsMonotonic() { if sum.AggregationTemporality() == pmetric.MetricAggregationTemporalityCumulative { metricIdentifier := datapointstorage.Identifier(resource, extraLabels, metric, point.Attributes()) @@ -761,7 +780,7 @@ func (m *metricMapper) sumPointToTimeSeries( Value: value, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(metric.Name(), metric), + Type: name, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -781,6 +800,11 @@ func (m *metricMapper) gaugePointToTimeSeries( // Drop points without a value. return nil } + name, err := m.metricNameToType(metric.Name(), metric) + if err != nil { + m.obs.log.Debug("Unable to get metric name for gauge metric.", zap.Error(err), zap.Any("metric", metric)) + return nil + } metricKind := metricpb.MetricDescriptor_GAUGE value, valueType := numberDataPointToValue(point) @@ -796,7 +820,7 @@ func (m *metricMapper) gaugePointToTimeSeries( Value: value, }}, Metric: &metricpb.Metric{ - Type: m.metricNameToType(metric.Name(), metric), + Type: name, Labels: mergeLabels( attributesToLabels(point.Attributes()), extraLabels, @@ -816,13 +840,17 @@ func (m *metricMapper) getMetricNamePrefix(name string) string { } // metricNameToType maps OTLP metric name to GCM metric type (aka name) -func (m *metricMapper) metricNameToType(name string, metric pdata.Metric) string { - return path.Join(m.getMetricNamePrefix(name), m.cfg.MetricConfig.GetMetricName(name, metric)) +func (m *metricMapper) metricNameToType(name string, metric pdata.Metric) (string, error) { + metricName, err := m.cfg.MetricConfig.GetMetricName(name, metric) + if err != nil { + return "", err + } + return path.Join(m.getMetricNamePrefix(name), metricName), nil } // defaultGetMetricName does not (further) customize the baseName -func defaultGetMetricName(baseName string, _ pdata.Metric) string { - return baseName +func defaultGetMetricName(baseName string, _ pdata.Metric) (string, error) { + return baseName, nil } func numberDataPointToValue( @@ -955,40 +983,54 @@ func (m *metricMapper) labelDescriptors( return result } -// Returns (sum, count) metric names for a summary metric. -func summaryMetricNames(name string) (string, string) { - sumName := fmt.Sprintf("%s%s", name, SummarySumSuffix) - countName := fmt.Sprintf("%s%s", name, SummaryCountPrefix) - return sumName, countName +// Returns (sum, count, quantile) metric names for a summary metric. +func (m *metricMapper) summaryMetricNames(pm pdata.Metric) (string, string, string, error) { + sumName, err := m.metricNameToType(pm.Name()+SummarySumSuffix, pm) + if err != nil { + return "", "", "", err + } + countName, err := m.metricNameToType(pm.Name()+SummaryCountPrefix, pm) + if err != nil { + return "", "", "", err + } + quantileName, err := m.metricNameToType(pm.Name(), pm) + if err != nil { + return "", "", "", err + } + return sumName, countName, quantileName, nil } func (m *metricMapper) summaryMetricDescriptors( pm pdata.Metric, extraLabels labels, ) []*metricpb.MetricDescriptor { - sumName, countName := summaryMetricNames(pm.Name()) + sumName, countName, quantileName, err := m.summaryMetricNames(pm) + if err != nil { + m.obs.log.Debug("Failed to get metric name for metric. Dropping the metric.", zap.Error(err), zap.Any("metric", pm)) + return nil + } labels := m.labelDescriptors(pm, extraLabels) return []*metricpb.MetricDescriptor{ { - Type: m.metricNameToType(sumName, pm), + Type: sumName, Labels: labels, MetricKind: metricpb.MetricDescriptor_CUMULATIVE, ValueType: metricpb.MetricDescriptor_DOUBLE, Unit: pm.Unit(), Description: pm.Description(), - DisplayName: sumName, + DisplayName: pm.Name() + SummarySumSuffix, }, { - Type: m.metricNameToType(countName, pm), + Type: countName, Labels: labels, MetricKind: metricpb.MetricDescriptor_CUMULATIVE, ValueType: metricpb.MetricDescriptor_DOUBLE, Unit: pm.Unit(), Description: pm.Description(), - DisplayName: countName, + DisplayName: pm.Name() + SummaryCountPrefix, }, { - Type: m.metricNameToType(pm.Name(), pm), + Type: quantileName, Labels: append( labels, &label.LabelDescriptor{ @@ -1013,7 +1055,11 @@ func (m *metricMapper) metricDescriptor( return m.summaryMetricDescriptors(pm, extraLabels) } kind, typ := mapMetricPointKind(pm) - metricType := m.metricNameToType(pm.Name(), pm) + metricType, err := m.metricNameToType(pm.Name(), pm) + if err != nil { + m.obs.log.Debug("Failed to get metric name for metric. Dropping the metric descriptor.", zap.Error(err), zap.Any("metric", pm)) + return nil + } labels := m.labelDescriptors(pm, extraLabels) // Return nil for unsupported types. if kind == metricpb.MetricDescriptor_METRIC_KIND_UNSPECIFIED { diff --git a/exporter/collector/metrics_test.go b/exporter/collector/metrics_test.go index f6a0eabb5..beea4ff91 100644 --- a/exporter/collector/metrics_test.go +++ b/exporter/collector/metrics_test.go @@ -1409,9 +1409,11 @@ func TestMetricNameToType(t *testing.T) { mapper, shutdown := newTestMetricMapper() defer shutdown() metric := pdata.NewMetric() + got, err := mapper.metricNameToType("foo", metric) + assert.NoError(t, err) assert.Equal( t, - mapper.metricNameToType("foo", metric), + got, "workload.googleapis.com/foo", "Should use workload metric domain with default config", ) @@ -1922,7 +1924,9 @@ func TestKnownDomains(t *testing.T) { mapper.cfg.MetricConfig.KnownDomains = test.knownDomains } mapper.cfg.MetricConfig.Prefix = "prefix" - assert.Equal(t, test.metricType, mapper.metricNameToType(test.name, metric)) + metricType, err := mapper.metricNameToType(test.name, metric) + assert.NoError(t, err) + assert.Equal(t, test.metricType, metricType) }) } }