From e9e67a2ca23a0e576faedfd2254db9b93e6d9939 Mon Sep 17 00:00:00 2001 From: Timon Engelke Date: Mon, 11 Aug 2025 17:34:38 +0200 Subject: [PATCH] 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 --- pkg/metrics_store/metrics_writer.go | 7 +- pkg/metrics_store/metrics_writer_test.go | 84 ++++++++++++------------ pkg/metricshandler/metrics_handler.go | 2 +- 3 files changed, 47 insertions(+), 46 deletions(-) diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 1deec871..9034b0bf 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/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) { diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index d32cc45c..826e0335 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -275,6 +275,46 @@ func TestSanitizeHeaders(t *testing.T) { headers []string expectedHeaders []string }{ + { + name: "OpenMetricsText unique headers", + contentType: expfmt.NewFormat(expfmt.TypeOpenMetrics), + headers: []string{ + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo info", + "# HELP foo foo_help\n# TYPE foo stateset", + "# HELP foo foo_help\n# TYPE foo counter", + }, + expectedHeaders: []string{ + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo info", + "# HELP foo foo_help\n# TYPE foo stateset", + "# HELP foo foo_help\n# TYPE foo counter", + }, + }, + { + name: "OpenMetricsText consecutive duplicate headers", + contentType: expfmt.NewFormat(expfmt.TypeOpenMetrics), + headers: []string{ + "", + "", + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo info", + "# HELP foo foo_help\n# TYPE foo info", + "# HELP foo foo_help\n# TYPE foo stateset", + "# HELP foo foo_help\n# TYPE foo stateset", + "# HELP foo foo_help\n# TYPE foo counter", + "# HELP foo foo_help\n# TYPE foo counter", + }, + expectedHeaders: []string{ + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo info", + "# HELP foo foo_help\n# TYPE foo stateset", + "# HELP foo foo_help\n# TYPE foo counter", + }, + }, { name: "text-format unique headers", contentType: expfmt.NewFormat(expfmt.TypeTextPlain), @@ -287,8 +327,6 @@ func TestSanitizeHeaders(t *testing.T) { }, expectedHeaders: []string{ "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo info", - "# HELP foo foo_help\n# TYPE foo stateset", "# HELP foo foo_help\n# TYPE foo counter", }, }, @@ -308,44 +346,6 @@ func TestSanitizeHeaders(t *testing.T) { "# HELP foo foo_help\n# TYPE foo counter", "# HELP foo foo_help\n# TYPE foo counter", }, - expectedHeaders: []string{ - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo info", - "# HELP foo foo_help\n# TYPE foo stateset", - "# HELP foo foo_help\n# TYPE foo counter", - }, - }, - { - name: "proto-format unique headers", - contentType: expfmt.NewFormat(expfmt.TypeProtoText), // Prometheus ProtoFmt is the only proto-based format we check for. - headers: []string{ - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo info", - "# HELP foo foo_help\n# TYPE foo stateset", - "# HELP foo foo_help\n# TYPE foo counter", - }, - expectedHeaders: []string{ - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo counter", - }, - }, - { - name: "proto-format consecutive duplicate headers", - contentType: expfmt.NewFormat(expfmt.TypeProtoText), // Prometheus ProtoFmt is the only proto-based format we check for. - headers: []string{ - "", - "", - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo info", - "# HELP foo foo_help\n# TYPE foo info", - "# HELP foo foo_help\n# TYPE foo stateset", - "# HELP foo foo_help\n# TYPE foo stateset", - "# HELP foo foo_help\n# TYPE foo counter", - "# HELP foo foo_help\n# TYPE foo counter", - }, expectedHeaders: []string{ "# HELP foo foo_help\n# TYPE foo gauge", "# HELP foo foo_help\n# TYPE foo counter", @@ -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}) } }) } diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go index 0a689147..8e1dfefa 100644 --- a/pkg/metricshandler/metrics_handler.go +++ b/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 {