Skip to content

Prometheus Receiver metric type fixes to match Prometheus functionality #3853

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.

Sign up for GitHub

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

Closed
gracewehner wants to merge 2 commits into from
Closed

Prometheus Receiver metric type fixes to match Prometheus functionality #3853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aef5dd0
metric typing
gracewehner Aug 17, 2021
b89198a
Add test case for where metadata key excludes _total but metric name …
gracewehner Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion receiver/prometheusreceiver/internal/metricfamily.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,20 @@ func newMetricFamily(metricName string, mc MetadataCache, logger *zap.Logger, in
}
} else if !ok && isInternalMetric(metricName) {
metadata = defineInternalMetric(metricName, metadata, logger)
} else if !ok {
// Prometheus sends metrics without a type hint as gauges.
// MetricTypeUnknown is converted a gauge in convToOCAMetricType()
logger.Debug(fmt.Sprintf("Metadata unknown, using Gauge type for: %s %+v", metricName, metadata))
metadata.Type = textparse.MetricTypeUnknown
}

ocaMetricType := convToOCAMetricType(metadata.Type)
if ocaMetricType == metricspb.MetricDescriptor_UNSPECIFIED {

// If a counter has a _total suffix but metadata is stored without it, keep _total suffix as the name otherwise
// the metric sent won't have the suffix
if ocaMetricType == metricspb.MetricDescriptor_CUMULATIVE_DOUBLE && strings.HasSuffix(metricName, metricSuffixTotal) {
familyName = metricName
} else if ocaMetricType == metricspb.MetricDescriptor_UNSPECIFIED {
logger.Debug(fmt.Sprintf("Invalid metric : %s %+v", metricName, metadata))
}

Expand Down
35 changes: 33 additions & 2 deletions receiver/prometheusreceiver/internal/metricsbuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,37 @@ func Test_metricBuilder_counters(t *testing.T) {
},
},
},
// Some counters such as "python_gc_collections_total" have metadata key as "python_gc_collections" but still need
// to be converted using full metric name as "python_gc_collections_total" to match Prometheus functionality
{
name: "counter-with-metadata-without-total-suffix",
inputs: []*testScrapedPage{
{
pts: []*testDataPoint{
createDataPoint("counter_test_total", 100, "foo", "bar"),
},
},
},
wants: [][]*metricspb.Metric{
{
{
MetricDescriptor: &metricspb.MetricDescriptor{
Name: "counter_test_total",
Type: metricspb.MetricDescriptor_CUMULATIVE_DOUBLE,
LabelKeys: []*metricspb.LabelKey{{Key: "foo"}}},
Timeseries: []*metricspb.TimeSeries{
{
StartTimestamp: timestampFromMs(startTs),
LabelValues: []*metricspb.LabelValue{{Value: "bar", HasValue: true}},
Points: []*metricspb.Point{
{Timestamp: timestampFromMs(startTs), Value: &metricspb.Point_DoubleValue{DoubleValue: 100.0}},
},
},
},
},
},
},
},
{
name: "two-items",
inputs: []*testScrapedPage{
Expand Down Expand Up @@ -544,7 +575,7 @@ func Test_metricBuilder_untype(t *testing.T) {
{
MetricDescriptor: &metricspb.MetricDescriptor{
Name: "something_not_exists",
Type: metricspb.MetricDescriptor_UNSPECIFIED,
Type: metricspb.MetricDescriptor_GAUGE_DOUBLE,
Copy link
Copy Markdown
Contributor

@dashpole dashpole Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually see where you set the type to GAUGE_DOUBLE anywhere in this PR. Is this a work in progress, or did you forget to commit some changes?

Copy link
Copy Markdown
Author

@gracewehner gracewehner Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is a bit hidden. This change sets the metadata.Type to textparse.MetricTypeUnknown which is already being done in line 66 here, just only in the situation where the metric name is not equal to the family name but the metadata still cannot be found:

opentelemetry-collector/receiver/prometheusreceiver/internal/metricfamily.go

Lines 56 to 67 in 8ac1fd7

metadata, ok := mc.Metadata(familyName)
if !ok && metricName != familyName {
// use the original metricName as metricFamily
familyName = metricName
// perform a 2nd lookup with the original metric name. it can happen if there's a metric which is not histogram
// or summary, but ends with one of those _count/_sum suffixes
metadata, ok = mc.Metadata(metricName)
// still not found, this can happen when metric has no TYPE HINT
if !ok {
metadata.Metric = familyName
metadata.Type = textparse.MetricTypeUnknown
}

The function convToOCAMetricType() is converting the textparse.MetricTypeUnknown to a GAUGE_DOUBLE, whereas before the change it was defaulting to UNSPECIFIED:

opentelemetry-collector/receiver/prometheusreceiver/internal/metricsbuilder.go

Lines 258 to 278 in 8739b2b

func convToOCAMetricType(metricType textparse.MetricType) metricspb.MetricDescriptor_Type {
switch metricType {
case textparse.MetricTypeCounter:
// always use float64, as it's the internal data type used in prometheus
return metricspb.MetricDescriptor_CUMULATIVE_DOUBLE
// textparse.MetricTypeUnknown is converted to gauge by default to fix Prometheus untyped metrics from being dropped
case textparse.MetricTypeGauge, textparse.MetricTypeUnknown:
return metricspb.MetricDescriptor_GAUGE_DOUBLE
case textparse.MetricTypeHistogram:
return metricspb.MetricDescriptor_CUMULATIVE_DISTRIBUTION
// dropping support for gaugehistogram for now until we have an official spec of its implementation
// a draft can be found in: https://docs.google.com/document/d/1KwV0mAXwwbvvifBvDKH_LU1YjyXE_wxCkHNoCGq1GX0/edit#heading=h.1cvzqd4ksd23
// case textparse.MetricTypeGaugeHistogram:
// return metricspb.MetricDescriptor_GAUGE_DISTRIBUTION
case textparse.MetricTypeSummary:
return metricspb.MetricDescriptor_SUMMARY
default:
// including: textparse.MetricTypeInfo, textparse.MetricTypeStateset
return metricspb.MetricDescriptor_UNSPECIFIED
}
}

LabelKeys: []*metricspb.LabelKey{{Key: "foo"}}},
Timeseries: []*metricspb.TimeSeries{
{
Expand All @@ -558,7 +589,7 @@ func Test_metricBuilder_untype(t *testing.T) {
{
MetricDescriptor: &metricspb.MetricDescriptor{
Name: "theother_not_exists",
Type: metricspb.MetricDescriptor_UNSPECIFIED,
Type: metricspb.MetricDescriptor_GAUGE_DOUBLE,
LabelKeys: []*metricspb.LabelKey{{Key: "bar"}, {Key: "foo"}}},
Timeseries: []*metricspb.TimeSeries{
{
Expand Down