Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove _total from duplicate untyped Sums before export #678

Merged
merged 2 commits into from Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 18 additions & 5 deletions exporter/collector/googlemanagedprometheus/naming.go
Expand Up @@ -17,6 +17,7 @@ package googlemanagedprometheus
import (
"fmt"
"strings"
"unicode"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheus"
"go.opentelemetry.io/collector/pdata/pmetric"
Expand All @@ -32,9 +33,9 @@ func GetMetricName(baseName string, metric pmetric.Metric) (string, error) {
// Non-monotonic sums are converted to GCM gauges
return compliantName + "/gauge", nil
}
return compliantName + getUnknownMetricSuffix(metric.Sum().DataPoints(), "/counter", "counter"), nil
return getUnknownMetricName(metric.Sum().DataPoints(), "/counter", "counter", metric.Name(), compliantName), nil
case pmetric.MetricTypeGauge:
return compliantName + getUnknownMetricSuffix(metric.Gauge().DataPoints(), "/gauge", ""), nil
return getUnknownMetricName(metric.Gauge().DataPoints(), "/gauge", "", metric.Name(), compliantName), nil
case pmetric.MetricTypeSummary:
// summaries are sent as the following series:
// * Sum: prometheus.googleapis.com/<baseName>_sum/summary:counter
Expand All @@ -56,11 +57,23 @@ func GetMetricName(baseName string, metric pmetric.Metric) (string, error) {

// getUnknownMetricSuffix will set the metric suffix for untyped metrics to
// "/unknown" (eg, for Gauge) or "/unknown:{secondarySuffix}" (eg, "/unknown:counter" for Sum).
// It also removes the "_total" suffix on an unknown counter, if this suffix was not present in
// the original metric name before calling prometheus.BuildCompliantName(), which is hacky.
// It is based on the untyped_prometheus_metric data point attribute, and behind a feature gate.
func getUnknownMetricSuffix(points pmetric.NumberDataPointSlice, suffix, secondarySuffix string) string {
func getUnknownMetricName(points pmetric.NumberDataPointSlice, suffix, secondarySuffix, originalName, compliantName string) string {
if !untypedDoubleExportFeatureGate.IsEnabled() {
return suffix
return compliantName + suffix
}

// de-normalize "_total" suffix for counters where not present on original metric name
nameTokens := strings.FieldsFunc(
dashpole marked this conversation as resolved.
Show resolved Hide resolved
originalName,
func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsDigit(r) },
)
if nameTokens[len(nameTokens)-1] != "total" && strings.HasSuffix(compliantName, "_total") {
compliantName = strings.TrimSuffix(compliantName, "_total")
}

newSuffix := suffix
for i := 0; i < points.Len(); i++ {
point := points.At(i)
Expand All @@ -74,5 +87,5 @@ func getUnknownMetricSuffix(points pmetric.NumberDataPointSlice, suffix, seconda
// even though we have the suffix, keep looping to remove the attribute from other points, if any
}
}
return newSuffix
return compliantName + newSuffix
}
15 changes: 15 additions & 0 deletions exporter/collector/googlemanagedprometheus/naming_test.go
Expand Up @@ -190,6 +190,21 @@ func TestGetMetricName(t *testing.T) {
},
expected: "bar/unknown:counter",
},
{
desc: "untyped sum with feature gate enabled + name normalization returns unknown:counter",
baseName: "bar",
metric: func(m pmetric.Metric) {
//nolint:errcheck
featuregate.GlobalRegistry().Set(gcpUntypedDoubleExportGateKey, true)
//nolint:errcheck
featuregate.GlobalRegistry().Set("pkg.translator.prometheus.NormalizeName", true)
m.SetName("bar")
m.SetEmptySum()
m.Sum().SetIsMonotonic(true)
m.Sum().DataPoints().AppendEmpty().Attributes().PutStr(GCPOpsAgentUntypedMetricKey, "true")
},
expected: "bar/unknown:counter",
},
} {
t.Run(tc.desc, func(t *testing.T) {
assert.True(t, gate.IsEnabled())
Expand Down
Expand Up @@ -365,5 +365,37 @@
// SDK exporter does not support CreateServiceTimeSeries
SkipForSDK: true,
},
{
// https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/issues/677
dashpole marked this conversation as resolved.
Show resolved Hide resolved
Name: "Google Managed Prometheus with Double Export for Untyped with name normalization enabled",
OTLPInputFixturePath: "testdata/fixtures/metrics/google_managed_prometheus.json",
ExpectFixturePath: "testdata/fixtures/metrics/google_managed_prometheus_untyped_name_normalized_expect.json",
ConfigureCollector: func(cfg *collector.Config) {
cfg.MetricConfig.Prefix = "prometheus.googleapis.com/"
cfg.MetricConfig.SkipCreateMetricDescriptor = true
cfg.MetricConfig.GetMetricName = googlemanagedprometheus.GetMetricName
cfg.MetricConfig.MapMonitoredResource = googlemanagedprometheus.MapToPrometheusTarget
cfg.MetricConfig.ExtraMetrics = func(m pmetric.Metrics) pmetric.ResourceMetricsSlice {
//nolint:errcheck
featuregate.GlobalRegistry().Set("gcp.untyped_double_export", true)
//nolint:errcheck
featuregate.GlobalRegistry().Set("pkg.translator.prometheus.NormalizeName", true)
googlemanagedprometheus.AddUntypedMetrics(m)
googlemanagedprometheus.AddScopeInfoMetric(m)
googlemanagedprometheus.AddTargetInfoMetric(m)
return m.ResourceMetrics()
}
cfg.MetricConfig.InstrumentationLibraryLabels = false
cfg.MetricConfig.ServiceResourceLabels = false
cfg.MetricConfig.EnableSumOfSquaredDeviation = true
// disable cumulative normalization so we can see the counter without having to send 2 data points.
// with normalization enabled, the first data point would get dropped.
// but trying to send 2 data points causes the GCM integration test to fail for duplicate timeseries.
cfg.MetricConfig.CumulativeNormalization = false

Check warning on line 394 in exporter/collector/integrationtest/testcases/testcases_metrics.go

View check run for this annotation

Codecov / codecov/patch

exporter/collector/integrationtest/testcases/testcases_metrics.go#L373-L394

Added lines #L373 - L394 were not covered by tests
},
// prometheus_target is not supported by the SDK
SkipForSDK: true,
Skip: true,
},
// TODO: Add integration tests for workload.googleapis.com metrics from the ops agent
}