fix: fix logic for plain text fallback format

This pull requests fixes a logic error in metrics_writer.go where metrics
headers are replaced when a protobuf format is requested. However, the
existing logic is never used because the content type negotiation is already
done in a previous step (in metrics_handler.go). There, the content type for
proto-based formats is changed to text/plain before passing the argument
to SanitizeHeaders.

The pull request changes the condition in SanitizeHeaders to check for
the plain-text format instead of protobuf. I changed the signature of
SanitizeHeaders to accept expfmt.Format instead of string. This makes checking
the content type a bit cleaner. If this is considered a breaking change, we
can also change it to a string prefix comparison.

I encountered the error when I tried to use native histogram parsing in
prometheus and found errors while parsing kube-state-metrics' metrics.
The issue is already described in #2587.

Signed-off-by: Timon Engelke <[email protected]>

Author: timonegk

pkg/metrics_store/metrics_writer.go

@@ -92,7 +92,7 @@ func (m MetricsWriter) WriteAll(w io.Writer) error {
 }
 
 // SanitizeHeaders sanitizes the headers of the given MetricsWriterList.
-func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWriterList {
+func SanitizeHeaders(contentType expfmt.Format, writers MetricsWriterList) MetricsWriterList {
 	var lastHeader string
 	for _, writer := range writers {
 		if len(writer.stores) > 0 {
@@ -104,8 +104,9 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite
 				// Skip this step if we encounter a repeated header, as it will be removed.
 				if header != lastHeader && strings.HasPrefix(header, "# HELP") {
 
-					// If the requested content type was proto-based (such as FmtProtoDelim, FmtProtoText, or FmtProtoCompact), replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' protobuf machinery.
-					if strings.HasPrefix(contentType, expfmt.ProtoType) {
+					// If the requested content type is text/plain, replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' plain text machinery.
+					// When Prometheus requests proto-based formats, this branch is also used because any requested format that is not OpenMetrics falls back to text/plain in metrics_handler.go
+					if contentType.FormatType() == expfmt.TypeTextPlain {
 						infoTypeString := string(metric.Info)
 						stateSetTypeString := string(metric.StateSet)
 						if strings.HasSuffix(header, infoTypeString) {

pkg/metrics_store/metrics_writer_test.go

@@ -276,8 +276,8 @@ func TestSanitizeHeaders(t *testing.T) {
 		expectedHeaders []string
 	}{
 		{
-			name:        "text-format unique headers",
-			contentType: expfmt.NewFormat(expfmt.TypeTextPlain),
+			name:        "OpenMetricsText unique headers",
+			contentType: expfmt.NewFormat(expfmt.TypeOpenMetrics),
 			headers: []string{
 				"",
 				"# HELP foo foo_help\n# TYPE foo gauge",
@@ -293,8 +293,8 @@ func TestSanitizeHeaders(t *testing.T) {
 			},
 		},
 		{
-			name:        "text-format consecutive duplicate headers",
-			contentType: expfmt.NewFormat(expfmt.TypeTextPlain),
+			name:        "OpenMetricsText consecutive duplicate headers",
+			contentType: expfmt.NewFormat(expfmt.TypeOpenMetrics),
 			headers: []string{
 				"",
 				"",
@@ -316,8 +316,8 @@ func TestSanitizeHeaders(t *testing.T) {
 			},
 		},
 		{
-			name:        "proto-format unique headers",
-			contentType: expfmt.NewFormat(expfmt.TypeProtoText), // Prometheus ProtoFmt is the only proto-based format we check for.
+			name:        "text-format unique headers",
+			contentType: expfmt.NewFormat(expfmt.TypeTextPlain),
 			headers: []string{
 				"",
 				"# HELP foo foo_help\n# TYPE foo gauge",
@@ -331,8 +331,8 @@ func TestSanitizeHeaders(t *testing.T) {
 			},
 		},
 		{
-			name:        "proto-format consecutive duplicate headers",
-			contentType: expfmt.NewFormat(expfmt.TypeProtoText), // Prometheus ProtoFmt is the only proto-based format we check for.
+			name:        "text-format consecutive duplicate headers",
+			contentType: expfmt.NewFormat(expfmt.TypeTextPlain),
 			headers: []string{
 				"",
 				"",
@@ -356,7 +356,7 @@ func TestSanitizeHeaders(t *testing.T) {
 	for _, testcase := range testcases {
 		writer := NewMetricsWriter(NewMetricsStore(testcase.headers, nil))
 		t.Run(testcase.name, func(t *testing.T) {
-			SanitizeHeaders(string(testcase.contentType), MetricsWriterList{writer})
+			SanitizeHeaders(testcase.contentType, MetricsWriterList{writer})
 			if !reflect.DeepEqual(testcase.expectedHeaders, writer.stores[0].headers) {
 				t.Fatalf("(-want, +got):\n%s", cmp.Diff(testcase.expectedHeaders, writer.stores[0].headers))
 			}
@@ -404,7 +404,7 @@ func BenchmarkSanitizeHeaders(b *testing.B) {
 		writer := NewMetricsWriter(NewMetricsStore(headers, nil))
 		b.Run(benchmark.name, func(b *testing.B) {
 			for i := 0; i < b.N; i++ {
-				SanitizeHeaders(string(benchmark.contentType), MetricsWriterList{writer})
+				SanitizeHeaders(benchmark.contentType, MetricsWriterList{writer})
 			}
 		})
 	}

pkg/metricshandler/metrics_handler.go

@@ -220,7 +220,7 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	m.metricsWriters = metricsstore.SanitizeHeaders(string(contentType), m.metricsWriters)
+	m.metricsWriters = metricsstore.SanitizeHeaders(contentType, m.metricsWriters)
 	for _, w := range m.metricsWriters {
 		err := w.WriteAll(writer)
 		if err != nil {