From ee2f53a2f103ad061b2e80182c07e25b6980d439 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 13 Dec 2023 12:37:50 +0530 Subject: [PATCH 01/10] fix: fallback to `gauge` for `protofmt`-based negotiations Fallback to `gauge` metric type when the negotiated content-type is `protofmt`-based, since Prometheus' protobuf machinery does not recognize all OpenMetrics types (`info` and `statesets` in this context). Signed-off-by: Pranshu Srivastava --- pkg/metrics_store/metrics_writer.go | 20 ++++++++-- pkg/metrics_store/metrics_writer_test.go | 47 ++++++++++++++++++++++++ pkg/metricshandler/metrics_handler.go | 3 +- 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 77a9df6106..8a99705af1 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -19,6 +19,11 @@ package metricsstore import ( "fmt" "io" + "strings" + + "github.com/prometheus/common/expfmt" + + "k8s.io/kube-state-metrics/v2/pkg/metric" ) // MetricsWriterList represent a list of MetricsWriter @@ -82,13 +87,22 @@ func (m MetricsWriter) WriteAll(w io.Writer) error { return nil } -// SanitizeHeaders removes duplicate headers from the given MetricsWriterList for the same family (generated through CRS). -// These are expected to be consecutive since G** resolution generates groups of similar metrics with same headers before moving onto the next G** spec in the CRS configuration. -func SanitizeHeaders(writers MetricsWriterList) MetricsWriterList { +// SanitizeHeaders sanitizes the headers of the given MetricsWriterList. +func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWriterList { var lastHeader string for _, writer := range writers { if len(writer.stores) > 0 { for i, header := range writer.stores[0].headers { + // If the requested content type was proto-based, replace the type with "gauge", as "info" and "statesets" are not recognized by Prometheus' protobuf machinery. + if strings.HasPrefix(contentType, expfmt.ProtoType) && + strings.HasPrefix(header, "# HELP") && + (strings.HasSuffix(header, " "+string(metric.Info)) || strings.HasSuffix(header, " "+string(metric.StateSet))) { + typeStringWithoutTypePaddedIndex := strings.LastIndex(header, " ") + typeStringWithoutType := header[:typeStringWithoutTypePaddedIndex] + writer.stores[0].headers[i] = typeStringWithoutType + " " + string(metric.Gauge) + } + // Removes duplicate headers from the given MetricsWriterList for the same family (generated through CRS). + // These are expected to be consecutive since G** resolution generates groups of similar metrics with same headers before moving onto the next G** spec in the CRS configuration. if header == lastHeader { writer.stores[0].headers[i] = "" } else { diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index 73bf164765..d7e94c89de 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -18,6 +18,7 @@ package metricsstore_test import ( "fmt" + "github.com/prometheus/common/expfmt" "strings" "testing" @@ -263,3 +264,49 @@ func TestWriteAllWithEmptyStores(t *testing.T) { t.Fatalf("Unexpected output, got %q, want %q", result, "") } } + +func BenchmarkSanitizeHeaders(b *testing.B) { + benchmarks := []struct { + name string + contentType expfmt.Format + writersContainsDuplicates bool + }{ + { + name: "text-format unique headers", + contentType: expfmt.FmtText, + writersContainsDuplicates: false, + }, + { + name: "text-format duplicate headers", + contentType: expfmt.FmtText, + writersContainsDuplicates: true, + }, + { + name: "proto-format unique headers", + contentType: expfmt.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. + writersContainsDuplicates: false, + }, + { + name: "proto-format duplicate headers", + contentType: expfmt.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. + writersContainsDuplicates: true, + }, + } + + for _, benchmark := range benchmarks { + headers := []string{} + for j := 0; j < 10e4; j++ { + if benchmark.writersContainsDuplicates { + headers = append(headers, fmt.Sprintf("# HELP foo foo_help\n# TYPE foo info")) + } else { + headers = append(headers, fmt.Sprintf("# HELP foo_%d foo_help\n# TYPE foo_%d info", j, j)) + } + } + writer := metricsstore.NewMetricsWriter(metricsstore.NewMetricsStore(headers, nil)) + b.Run(benchmark.name, func(b *testing.B) { + for i := 0; i < b.N; i++ { + metricsstore.SanitizeHeaders(string(benchmark.contentType), metricsstore.MetricsWriterList{writer}) + } + }) + } +} diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go index 911d24b161..4f2ddb530b 100644 --- a/pkg/metricshandler/metrics_handler.go +++ b/pkg/metricshandler/metrics_handler.go @@ -187,6 +187,7 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { var writer io.Writer = w contentType := expfmt.NegotiateIncludingOpenMetrics(r.Header) + gotContentType := contentType // We do not support protobuf at the moment. Fall back to FmtText if the negotiated exposition format is not FmtOpenMetrics See: https://github.com/kubernetes/kube-state-metrics/issues/2022 if contentType != expfmt.FmtOpenMetrics_1_0_0 && contentType != expfmt.FmtOpenMetrics_0_0_1 { @@ -208,7 +209,7 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - m.metricsWriters = metricsstore.SanitizeHeaders(m.metricsWriters) + m.metricsWriters = metricsstore.SanitizeHeaders(string(gotContentType), m.metricsWriters) for _, w := range m.metricsWriters { err := w.WriteAll(writer) if err != nil { From 4be2ddf3f7027e0808457087d8b2ceac66d892f3 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Fri, 15 Dec 2023 17:39:26 +0530 Subject: [PATCH 02/10] fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/customresourcestate/config.go | 22 +++++- .../config_metrics_types.go | 10 --- .../custom_resource_metrics_test.go | 8 ++- pkg/customresourcestate/registry_factory.go | 8 +-- pkg/metric/metric.go | 67 ++++++++++++++++--- pkg/metric_generator/generator.go | 2 +- pkg/metrics_store/metrics_writer.go | 27 +++++--- pkg/metrics_store/metrics_writer_test.go | 5 +- 8 files changed, 110 insertions(+), 39 deletions(-) diff --git a/pkg/customresourcestate/config.go b/pkg/customresourcestate/config.go index 385858b8ba..aaab8d8c83 100644 --- a/pkg/customresourcestate/config.go +++ b/pkg/customresourcestate/config.go @@ -20,6 +20,8 @@ import ( "fmt" "strings" + "k8s.io/kube-state-metrics/v2/pkg/metric" + "github.com/gobuffalo/flect" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/klog/v2" @@ -148,7 +150,7 @@ type Generator struct { type Metric struct { // Type defines the type of the metric. // +unionDiscriminator - Type MetricType `yaml:"type" json:"type"` + Type metric.Type `yaml:"type" json:"type"` // Gauge defines a gauge metric. // +optional @@ -170,9 +172,16 @@ type ConfigDecoder interface { func FromConfig(decoder ConfigDecoder, discovererInstance *discovery.CRDiscoverer) (func() ([]customresource.RegistryFactory, error), error) { var customResourceConfig Metrics factoriesIndex := map[string]bool{} + + // Decode the configuration. if err := decoder.Decode(&customResourceConfig); err != nil { return nil, fmt.Errorf("failed to parse Custom Resource State metrics: %w", err) } + + // Override the configuration with any custom overrides. + configOverrides(&customResourceConfig) + + // Create a factory for each resource. fn := func() (factories []customresource.RegistryFactory, err error) { resources := customResourceConfig.Spec.Resources // resolvedGVKPs will have the final list of GVKs, in addition to the resolved G** resources. @@ -206,3 +215,14 @@ func FromConfig(decoder ConfigDecoder, discovererInstance *discovery.CRDiscovere } return fn, nil } + +// configOverrides applies overrides to the configuration. +func configOverrides(config *Metrics) { + for i := range config.Spec.Resources { + for j := range config.Spec.Resources[i].Metrics { + + // Override the metric type to lowercase, so the internals have a single source of truth for metric type definitions. + config.Spec.Resources[i].Metrics[j].Each.Type = metric.Type(strings.ToLower(string(config.Spec.Resources[i].Metrics[j].Each.Type))) + } + } +} diff --git a/pkg/customresourcestate/config_metrics_types.go b/pkg/customresourcestate/config_metrics_types.go index 6e8e9167cd..25d63bfb69 100644 --- a/pkg/customresourcestate/config_metrics_types.go +++ b/pkg/customresourcestate/config_metrics_types.go @@ -16,16 +16,6 @@ limitations under the License. package customresourcestate -// MetricType is the type of a metric. -type MetricType string - -// Supported metric types. -const ( - MetricTypeGauge MetricType = "Gauge" - MetricTypeStateSet MetricType = "StateSet" - MetricTypeInfo MetricType = "Info" -) - // MetricMeta are variables which may used for any metric type. type MetricMeta struct { // LabelsFromPath adds additional labels where the value of the label is taken from a field under Path. diff --git a/pkg/customresourcestate/custom_resource_metrics_test.go b/pkg/customresourcestate/custom_resource_metrics_test.go index 928f668ddd..31121de75c 100644 --- a/pkg/customresourcestate/custom_resource_metrics_test.go +++ b/pkg/customresourcestate/custom_resource_metrics_test.go @@ -21,6 +21,8 @@ import ( "reflect" "testing" + "k8s.io/kube-state-metrics/v2/pkg/metric" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" ) @@ -55,7 +57,7 @@ func TestNewCustomResourceMetrics(t *testing.T) { Name: "test_metrics", Help: "metrics for testing", Each: Metric{ - Type: MetricTypeInfo, + Type: metric.Info, Info: &MetricInfo{ MetricMeta: MetricMeta{ Path: []string{ @@ -117,7 +119,7 @@ func TestNewCustomResourceMetrics(t *testing.T) { Name: "test_metrics", Help: "metrics for testing", Each: Metric{ - Type: MetricTypeInfo, + Type: metric.Info, Info: &MetricInfo{ MetricMeta: MetricMeta{ Path: []string{ @@ -180,7 +182,7 @@ func TestNewCustomResourceMetrics(t *testing.T) { Name: "test_metrics", Help: "metrics for testing", Each: Metric{ - Type: MetricTypeInfo, + Type: metric.Info, Info: &MetricInfo{ MetricMeta: MetricMeta{ Path: []string{ diff --git a/pkg/customresourcestate/registry_factory.go b/pkg/customresourcestate/registry_factory.go index a1ea43f962..d5f920e466 100644 --- a/pkg/customresourcestate/registry_factory.go +++ b/pkg/customresourcestate/registry_factory.go @@ -72,7 +72,7 @@ func compileCommon(c MetricMeta) (*compiledCommon, error) { func compileFamily(f Generator, resource Resource) (*compiledFamily, error) { labels := resource.Labels.Merge(f.Labels) - if f.Each.Type == MetricTypeInfo && !strings.HasSuffix(f.Name, "_info") { + if f.Each.Type == metric.Info && !strings.HasSuffix(f.Name, "_info") { klog.InfoS("Info metric does not have _info suffix", "gvk", resource.GroupVersionKind.String(), "name", f.Name) } @@ -153,7 +153,7 @@ type compiledMetric interface { // newCompiledMetric returns a compiledMetric depending on the given metric type. func newCompiledMetric(m Metric) (compiledMetric, error) { switch m.Type { - case MetricTypeGauge: + case metric.Gauge: if m.Gauge == nil { return nil, errors.New("expected each.gauge to not be nil") } @@ -172,7 +172,7 @@ func newCompiledMetric(m Metric) (compiledMetric, error) { NilIsZero: m.Gauge.NilIsZero, labelFromKey: m.Gauge.LabelFromKey, }, nil - case MetricTypeInfo: + case metric.Info: if m.Info == nil { return nil, errors.New("expected each.info to not be nil") } @@ -185,7 +185,7 @@ func newCompiledMetric(m Metric) (compiledMetric, error) { compiledCommon: *cc, labelFromKey: m.Info.LabelFromKey, }, nil - case MetricTypeStateSet: + case metric.StateSet: if m.StateSet == nil { return nil, errors.New("expected each.stateSet to not be nil") } diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index 007eb9fa87..657bcbe29d 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -37,21 +37,68 @@ var ( } ) -// Type represents the type of a metric e.g. a counter. See -// https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-types. +// Type represents the type of the metric. See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-types. type Type string -// Gauge defines a OpenMetrics gauge. -var Gauge Type = "gauge" +// TypeN represents the type of the metric as an integer. +type TypeN int -// Info defines an OpenMetrics info. -var Info Type = "info" +// String returns the string representation of the metric type. +func (i TypeN) String() Type { + return MetricTypeNMap[i] +} + +// NString returns the string representation of the metric type as an integer. +func (i TypeN) NString() string { + return strconv.Itoa(int(i)) +} + +// Supported metric types. +const ( + // GaugeN defines a OpenMetrics gauge. + GaugeN TypeN = iota + + // InfoN defines an OpenMetrics info. + InfoN + + // StateSetN defines an OpenMetrics stateset. + StateSetN + + // CounterN defines a OpenMetrics counter. + CounterN +) -// StateSet defines an OpenMetrics stateset. -var StateSet Type = "stateset" +// Supported metric types. +var ( + + // Gauge defines a OpenMetrics gauge. + Gauge Type = "gauge" + + // Info defines an OpenMetrics info. + Info Type = "info" + + // StateSet defines an OpenMetrics stateset. + StateSet Type = "stateset" + + // Counter defines a OpenMetrics counter. + Counter Type = "counter" -// Counter defines a OpenMetrics counter. -var Counter Type = "counter" + // MetricTypeNMap is a map of MetricTypeN to MetricType. + MetricTypeNMap = map[TypeN]Type{ + GaugeN: Gauge, + InfoN: Info, + StateSetN: StateSet, + CounterN: Counter, + } + + // MetricTypeMap is a map of MetricType to MetricTypeN. + MetricTypeMap = map[Type]TypeN{ + Gauge: GaugeN, + Info: InfoN, + StateSet: StateSetN, + Counter: CounterN, + } +) // Metric represents a single time series. type Metric struct { diff --git a/pkg/metric_generator/generator.go b/pkg/metric_generator/generator.go index 2a2e30221f..4aaef6ce4e 100644 --- a/pkg/metric_generator/generator.go +++ b/pkg/metric_generator/generator.go @@ -90,7 +90,7 @@ func (g *FamilyGenerator) generateHeader() string { header.WriteString("# TYPE ") header.WriteString(g.Name) header.WriteByte(' ') - header.WriteString(string(g.Type)) + header.WriteString(metric.MetricTypeMap[g.Type].NString()) return header.String() } diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 8a99705af1..e50f730803 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -93,20 +93,31 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite for _, writer := range writers { if len(writer.stores) > 0 { for i, header := range writer.stores[0].headers { - // If the requested content type was proto-based, replace the type with "gauge", as "info" and "statesets" are not recognized by Prometheus' protobuf machinery. - if strings.HasPrefix(contentType, expfmt.ProtoType) && - strings.HasPrefix(header, "# HELP") && - (strings.HasSuffix(header, " "+string(metric.Info)) || strings.HasSuffix(header, " "+string(metric.StateSet))) { - typeStringWithoutTypePaddedIndex := strings.LastIndex(header, " ") - typeStringWithoutType := header[:typeStringWithoutTypePaddedIndex] - writer.stores[0].headers[i] = typeStringWithoutType + " " + string(metric.Gauge) + + // Nothing to sanitize. + if len(header) == 0 { + continue } + // Removes duplicate headers from the given MetricsWriterList for the same family (generated through CRS). // These are expected to be consecutive since G** resolution generates groups of similar metrics with same headers before moving onto the next G** spec in the CRS configuration. if header == lastHeader { writer.stores[0].headers[i] = "" - } else { + } else if strings.HasPrefix(header, "# HELP") { lastHeader = header + + // If the requested content type was proto-based, replace the type with "gauge", as "info" and "statesets" are not recognized by Prometheus' protobuf machinery, + // else replace them by their respective string representations. + if strings.HasPrefix(contentType, expfmt.ProtoType) && + (strings.HasSuffix(header, metric.InfoN.NString()) || strings.HasSuffix(header, metric.StateSetN.NString())) { + writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.Gauge) + } + + // Replace all remaining type enums with their string representations. + n := int(header[len(header)-1]) - '0' + if n >= 0 && n < len(metric.MetricTypeNMap) { + writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.MetricTypeNMap[metric.TypeN(n)]) + } } } } diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index d7e94c89de..5939ac6958 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -18,10 +18,11 @@ package metricsstore_test import ( "fmt" - "github.com/prometheus/common/expfmt" "strings" "testing" + "github.com/prometheus/common/expfmt" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -297,7 +298,7 @@ func BenchmarkSanitizeHeaders(b *testing.B) { headers := []string{} for j := 0; j < 10e4; j++ { if benchmark.writersContainsDuplicates { - headers = append(headers, fmt.Sprintf("# HELP foo foo_help\n# TYPE foo info")) + headers = append(headers, "# HELP foo foo_help\n# TYPE foo info") } else { headers = append(headers, fmt.Sprintf("# HELP foo_%d foo_help\n# TYPE foo_%d info", j, j)) } From bfa611dc51cb8792f6516a7477686693b05a3afb Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Tue, 19 Dec 2023 18:04:05 +0530 Subject: [PATCH 03/10] fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metric/metric.go | 20 ++++++++++---------- pkg/metric_generator/generator.go | 2 +- pkg/metrics_store/metrics_writer.go | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index 657bcbe29d..0ee6a40bd5 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -44,8 +44,8 @@ type Type string type TypeN int // String returns the string representation of the metric type. -func (i TypeN) String() Type { - return MetricTypeNMap[i] +func (i TypeN) String() string { + return string(TypeNMap[i]) } // NString returns the string representation of the metric type as an integer. @@ -55,7 +55,7 @@ func (i TypeN) NString() string { // Supported metric types. const ( - // GaugeN defines a OpenMetrics gauge. + // GaugeN defines an OpenMetrics gauge. GaugeN TypeN = iota // InfoN defines an OpenMetrics info. @@ -64,14 +64,14 @@ const ( // StateSetN defines an OpenMetrics stateset. StateSetN - // CounterN defines a OpenMetrics counter. + // CounterN defines an OpenMetrics counter. CounterN ) // Supported metric types. var ( - // Gauge defines a OpenMetrics gauge. + // Gauge defines an OpenMetrics gauge. Gauge Type = "gauge" // Info defines an OpenMetrics info. @@ -80,19 +80,19 @@ var ( // StateSet defines an OpenMetrics stateset. StateSet Type = "stateset" - // Counter defines a OpenMetrics counter. + // Counter defines an OpenMetrics counter. Counter Type = "counter" - // MetricTypeNMap is a map of MetricTypeN to MetricType. - MetricTypeNMap = map[TypeN]Type{ + // TypeNMap is a map of MetricTypeN to MetricType. + TypeNMap = map[TypeN]Type{ GaugeN: Gauge, InfoN: Info, StateSetN: StateSet, CounterN: Counter, } - // MetricTypeMap is a map of MetricType to MetricTypeN. - MetricTypeMap = map[Type]TypeN{ + // TypeMap is a map of MetricType to MetricTypeN. + TypeMap = map[Type]TypeN{ Gauge: GaugeN, Info: InfoN, StateSet: StateSetN, diff --git a/pkg/metric_generator/generator.go b/pkg/metric_generator/generator.go index 4aaef6ce4e..53065d3d1a 100644 --- a/pkg/metric_generator/generator.go +++ b/pkg/metric_generator/generator.go @@ -90,7 +90,7 @@ func (g *FamilyGenerator) generateHeader() string { header.WriteString("# TYPE ") header.WriteString(g.Name) header.WriteByte(' ') - header.WriteString(metric.MetricTypeMap[g.Type].NString()) + header.WriteString(metric.TypeMap[g.Type].NString()) return header.String() } diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index e50f730803..75daea06be 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -115,8 +115,8 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite // Replace all remaining type enums with their string representations. n := int(header[len(header)-1]) - '0' - if n >= 0 && n < len(metric.MetricTypeNMap) { - writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.MetricTypeNMap[metric.TypeN(n)]) + if n >= 0 && n < len(metric.TypeNMap) { + writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.TypeNMap[metric.TypeN(n)]) } } } From 67f184cb1395d6d44ba328ab3fc21d9ad49f3ea8 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 20 Dec 2023 05:03:40 +0530 Subject: [PATCH 04/10] fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/customresourcestate/registry_factory.go | 2 +- pkg/metric/metric.go | 43 +-------------------- pkg/metric_generator/generator.go | 2 +- pkg/metrics_store/metrics_writer.go | 21 +++++----- 4 files changed, 14 insertions(+), 54 deletions(-) diff --git a/pkg/customresourcestate/registry_factory.go b/pkg/customresourcestate/registry_factory.go index d5f920e466..907c786655 100644 --- a/pkg/customresourcestate/registry_factory.go +++ b/pkg/customresourcestate/registry_factory.go @@ -152,7 +152,7 @@ type compiledMetric interface { // newCompiledMetric returns a compiledMetric depending on the given metric type. func newCompiledMetric(m Metric) (compiledMetric, error) { - switch m.Type { + switch metric.Type(strings.ToLower(string(m.Type))) { case metric.Gauge: if m.Gauge == nil { return nil, errors.New("expected each.gauge to not be nil") diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index 0ee6a40bd5..47787fe2df 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -40,34 +40,11 @@ var ( // Type represents the type of the metric. See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-types. type Type string -// TypeN represents the type of the metric as an integer. -type TypeN int - // String returns the string representation of the metric type. -func (i TypeN) String() string { - return string(TypeNMap[i]) -} - -// NString returns the string representation of the metric type as an integer. -func (i TypeN) NString() string { - return strconv.Itoa(int(i)) +func (t Type) String() string { + return string(t) } -// Supported metric types. -const ( - // GaugeN defines an OpenMetrics gauge. - GaugeN TypeN = iota - - // InfoN defines an OpenMetrics info. - InfoN - - // StateSetN defines an OpenMetrics stateset. - StateSetN - - // CounterN defines an OpenMetrics counter. - CounterN -) - // Supported metric types. var ( @@ -82,22 +59,6 @@ var ( // Counter defines an OpenMetrics counter. Counter Type = "counter" - - // TypeNMap is a map of MetricTypeN to MetricType. - TypeNMap = map[TypeN]Type{ - GaugeN: Gauge, - InfoN: Info, - StateSetN: StateSet, - CounterN: Counter, - } - - // TypeMap is a map of MetricType to MetricTypeN. - TypeMap = map[Type]TypeN{ - Gauge: GaugeN, - Info: InfoN, - StateSet: StateSetN, - Counter: CounterN, - } ) // Metric represents a single time series. diff --git a/pkg/metric_generator/generator.go b/pkg/metric_generator/generator.go index 53065d3d1a..9442c6554e 100644 --- a/pkg/metric_generator/generator.go +++ b/pkg/metric_generator/generator.go @@ -90,7 +90,7 @@ func (g *FamilyGenerator) generateHeader() string { header.WriteString("# TYPE ") header.WriteString(g.Name) header.WriteByte(' ') - header.WriteString(metric.TypeMap[g.Type].NString()) + header.WriteString(g.Type.String()) return header.String() } diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 75daea06be..6e9386e18c 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -106,17 +106,16 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite } else if strings.HasPrefix(header, "# HELP") { lastHeader = header - // If the requested content type was proto-based, replace the type with "gauge", as "info" and "statesets" are not recognized by Prometheus' protobuf machinery, - // else replace them by their respective string representations. - if strings.HasPrefix(contentType, expfmt.ProtoType) && - (strings.HasSuffix(header, metric.InfoN.NString()) || strings.HasSuffix(header, metric.StateSetN.NString())) { - writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.Gauge) - } - - // Replace all remaining type enums with their string representations. - n := int(header[len(header)-1]) - '0' - if n >= 0 && n < len(metric.TypeNMap) { - writer.stores[0].headers[i] = header[:len(header)-1] + string(metric.TypeNMap[metric.TypeN(n)]) + // If the requested content type was proto-based, replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' protobuf machinery. + if strings.HasPrefix(contentType, expfmt.ProtoType) { + infoTypeString := metric.Info.String() + stateSetTypeString := metric.StateSet.String() + if strings.HasSuffix(header, infoTypeString) { + writer.stores[0].headers[i] = header[:len(header)-len(infoTypeString)] + string(metric.Gauge) + } + if strings.HasSuffix(header, stateSetTypeString) { + writer.stores[0].headers[i] = header[:len(header)-len(stateSetTypeString)] + string(metric.Gauge) + } } } } From 0e23fdbe5a905f4407a21cddfc49e8b96b1dfb90 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 20 Dec 2023 06:33:03 +0530 Subject: [PATCH 05/10] fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metrics_store/metrics_writer_test.go | 123 ++++++++++++++++++++--- 1 file changed, 110 insertions(+), 13 deletions(-) diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index 5939ac6958..b6fc7ff21c 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -14,21 +14,21 @@ See the License for the specific language governing permissions and limitations under the License. */ -package metricsstore_test +package metricsstore import ( "fmt" + "github.com/google/go-cmp/cmp" + "github.com/prometheus/common/expfmt" + "reflect" "strings" "testing" - "github.com/prometheus/common/expfmt" - v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/kube-state-metrics/v2/pkg/metric" - metricsstore "k8s.io/kube-state-metrics/v2/pkg/metrics_store" ) func TestWriteAllWithSingleStore(t *testing.T) { @@ -62,7 +62,7 @@ func TestWriteAllWithSingleStore(t *testing.T) { return []metric.FamilyInterface{&mf1, &mf2} } - store := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) + store := NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) svcs := []v1.Service{ { ObjectMeta: metav1.ObjectMeta{ @@ -86,7 +86,7 @@ func TestWriteAllWithSingleStore(t *testing.T) { } } - multiNsWriter := metricsstore.NewMetricsWriter(store) + multiNsWriter := NewMetricsWriter(store) w := strings.Builder{} err := multiNsWriter.WriteAll(&w) if err != nil { @@ -150,7 +150,7 @@ func TestWriteAllWithMultipleStores(t *testing.T) { return []metric.FamilyInterface{&mf1, &mf2} } - s1 := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) + s1 := NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) svcs1 := []v1.Service{ { ObjectMeta: metav1.ObjectMeta{ @@ -190,7 +190,7 @@ func TestWriteAllWithMultipleStores(t *testing.T) { }, }, } - s2 := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) + s2 := NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) for _, s := range svcs2 { svc := s if err := s2.Add(&svc); err != nil { @@ -198,7 +198,7 @@ func TestWriteAllWithMultipleStores(t *testing.T) { } } - multiNsWriter := metricsstore.NewMetricsWriter(s1, s2) + multiNsWriter := NewMetricsWriter(s1, s2) w := strings.Builder{} err := multiNsWriter.WriteAll(&w) if err != nil { @@ -250,9 +250,9 @@ func TestWriteAllWithEmptyStores(t *testing.T) { return []metric.FamilyInterface{&mf1, &mf2} } - store := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) + store := NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc) - multiNsWriter := metricsstore.NewMetricsWriter(store) + multiNsWriter := NewMetricsWriter(store) w := strings.Builder{} err := multiNsWriter.WriteAll(&w) if err != nil { @@ -266,6 +266,103 @@ func TestWriteAllWithEmptyStores(t *testing.T) { } } +// TODO: AFAIR empty headers are ignored by Prometheus? If not, we should remove them. +func TestSanitizeHeaders(t *testing.T) { + boilerplateHeaders := []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", + } + duplicatedBoilerplateHeaders := []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", + } + dedepedBoilerplateHeaders := []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", + "", + } + protoIngestibleHeaders := []string{ + "", + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo gauge", + "# HELP foo foo_help\n# TYPE foo counter", + } + dedepedProtoIngestibleHeaders := []string{ + "", + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "", + "# HELP foo foo_help\n# TYPE foo gauge", + "", + "# HELP foo foo_help\n# TYPE foo counter", + "", + } + testcases := []struct { + name string + contentType expfmt.Format + headers []string + expectedHeaders []string + }{ + { + name: "text-format unique headers", + contentType: expfmt.FmtText, + headers: boilerplateHeaders, + expectedHeaders: boilerplateHeaders, + }, + { + name: "text-format consecutive duplicate headers", + contentType: expfmt.FmtText, + headers: duplicatedBoilerplateHeaders, + expectedHeaders: dedepedBoilerplateHeaders, + }, + { + name: "proto-format unique headers", + contentType: expfmt.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. + headers: boilerplateHeaders, + expectedHeaders: protoIngestibleHeaders, + }, + { + name: "proto-format consecutive duplicate headers", + contentType: expfmt.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. + headers: duplicatedBoilerplateHeaders, + expectedHeaders: dedepedProtoIngestibleHeaders, + }, + } + + for _, testcase := range testcases { + writer := NewMetricsWriter(NewMetricsStore(testcase.headers, nil)) + t.Run(testcase.name, func(t *testing.T) { + SanitizeHeaders(string(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)) + } + }) + } +} + func BenchmarkSanitizeHeaders(b *testing.B) { benchmarks := []struct { name string @@ -303,10 +400,10 @@ func BenchmarkSanitizeHeaders(b *testing.B) { headers = append(headers, fmt.Sprintf("# HELP foo_%d foo_help\n# TYPE foo_%d info", j, j)) } } - writer := metricsstore.NewMetricsWriter(metricsstore.NewMetricsStore(headers, nil)) + writer := NewMetricsWriter(NewMetricsStore(headers, nil)) b.Run(benchmark.name, func(b *testing.B) { for i := 0; i < b.N; i++ { - metricsstore.SanitizeHeaders(string(benchmark.contentType), metricsstore.MetricsWriterList{writer}) + SanitizeHeaders(string(benchmark.contentType), MetricsWriterList{writer}) } }) } From 883296e9461aedcca62c7b6e8cb34fb4fe3b1078 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 20 Dec 2023 06:33:54 +0530 Subject: [PATCH 06/10] fixup! fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metrics_store/metrics_writer_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index b6fc7ff21c..890c1e0d6f 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -18,12 +18,13 @@ package metricsstore import ( "fmt" - "github.com/google/go-cmp/cmp" - "github.com/prometheus/common/expfmt" "reflect" "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/prometheus/common/expfmt" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -266,7 +267,8 @@ func TestWriteAllWithEmptyStores(t *testing.T) { } } -// TODO: AFAIR empty headers are ignored by Prometheus? If not, we should remove them. +// TODO: AFFAIR empty headers are ignored by Prometheus? If not, we should remove them. +// No two consecutive headers will be entirely the same. The cases used below are only for their suffixes. func TestSanitizeHeaders(t *testing.T) { boilerplateHeaders := []string{ "", From b6496c7fb4e6e2be8922278373fa71513f787f39 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 20 Dec 2023 14:45:17 +0530 Subject: [PATCH 07/10] fixup! fixup! fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metrics_store/metrics_writer.go | 32 +++++- pkg/metrics_store/metrics_writer_test.go | 137 +++++++++++------------ 2 files changed, 94 insertions(+), 75 deletions(-) diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 6e9386e18c..ff446e31ae 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -101,25 +101,45 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite // Removes duplicate headers from the given MetricsWriterList for the same family (generated through CRS). // These are expected to be consecutive since G** resolution generates groups of similar metrics with same headers before moving onto the next G** spec in the CRS configuration. - if header == lastHeader { - writer.stores[0].headers[i] = "" - } else if strings.HasPrefix(header, "# HELP") { - lastHeader = header + // 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, replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' protobuf machinery. if strings.HasPrefix(contentType, expfmt.ProtoType) { infoTypeString := metric.Info.String() stateSetTypeString := metric.StateSet.String() if strings.HasSuffix(header, infoTypeString) { - writer.stores[0].headers[i] = header[:len(header)-len(infoTypeString)] + string(metric.Gauge) + header = header[:len(header)-len(infoTypeString)] + string(metric.Gauge) + writer.stores[0].headers[i] = header } if strings.HasSuffix(header, stateSetTypeString) { - writer.stores[0].headers[i] = header[:len(header)-len(stateSetTypeString)] + string(metric.Gauge) + header = header[:len(header)-len(stateSetTypeString)] + string(metric.Gauge) + writer.stores[0].headers[i] = header } } } + + // Nullify duplicate headers after the sanitization to not miss out on any new candidates. + if header == lastHeader { + writer.stores[0].headers[i] = "" + } + + // Update the last header. + lastHeader = header } } } + + // Remove all empty headers. + for _, writer := range writers { + if len(writer.stores) > 0 { + for i := len(writer.stores[0].headers) - 1; i >= 0; i-- { + if writer.stores[0].headers[i] == "" { + writer.stores[0].headers = append(writer.stores[0].headers[:i], writer.stores[0].headers[i+1:]...) + } + } + } + } + return writers } diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index 890c1e0d6f..4d33a12369 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/pkg/metrics_store/metrics_writer_test.go @@ -267,61 +267,8 @@ func TestWriteAllWithEmptyStores(t *testing.T) { } } -// TODO: AFFAIR empty headers are ignored by Prometheus? If not, we should remove them. // No two consecutive headers will be entirely the same. The cases used below are only for their suffixes. func TestSanitizeHeaders(t *testing.T) { - boilerplateHeaders := []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", - } - duplicatedBoilerplateHeaders := []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", - } - dedepedBoilerplateHeaders := []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", - "", - } - protoIngestibleHeaders := []string{ - "", - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo gauge", - "# HELP foo foo_help\n# TYPE foo counter", - } - dedepedProtoIngestibleHeaders := []string{ - "", - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "", - "# HELP foo foo_help\n# TYPE foo gauge", - "", - "# HELP foo foo_help\n# TYPE foo counter", - "", - } testcases := []struct { name string contentType expfmt.Format @@ -329,28 +276,80 @@ func TestSanitizeHeaders(t *testing.T) { expectedHeaders []string }{ { - name: "text-format unique headers", - contentType: expfmt.FmtText, - headers: boilerplateHeaders, - expectedHeaders: boilerplateHeaders, + name: "text-format unique headers", + contentType: expfmt.FmtText, + 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: "text-format consecutive duplicate headers", - contentType: expfmt.FmtText, - headers: duplicatedBoilerplateHeaders, - expectedHeaders: dedepedBoilerplateHeaders, + name: "text-format consecutive duplicate headers", + contentType: expfmt.FmtText, + 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: "proto-format unique headers", - contentType: expfmt.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. - headers: boilerplateHeaders, - expectedHeaders: protoIngestibleHeaders, + name: "proto-format unique headers", + contentType: expfmt.ProtoFmt, // 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.ProtoFmt, // Prometheus ProtoFmt is the only proto-based format we check for. - headers: duplicatedBoilerplateHeaders, - expectedHeaders: dedepedProtoIngestibleHeaders, + name: "proto-format consecutive duplicate headers", + contentType: expfmt.ProtoFmt, // 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", + }, }, } From e0c3e8384aba73dc55257cdd3725c0276d4360bb Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Mon, 15 Jan 2024 01:49:38 +0530 Subject: [PATCH 08/10] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/customresourcestate/config.go | 1 + pkg/customresourcestate/config_test.go | 1 + pkg/customresourcestate/registry_factory.go | 2 +- pkg/metric/metric.go | 5 ---- pkg/metric_generator/generator.go | 2 +- pkg/metrics_store/metrics_writer.go | 29 +++++++-------------- pkg/metricshandler/metrics_handler.go | 4 +-- 7 files changed, 16 insertions(+), 28 deletions(-) diff --git a/pkg/customresourcestate/config.go b/pkg/customresourcestate/config.go index aaab8d8c83..873591372f 100644 --- a/pkg/customresourcestate/config.go +++ b/pkg/customresourcestate/config.go @@ -222,6 +222,7 @@ func configOverrides(config *Metrics) { for j := range config.Spec.Resources[i].Metrics { // Override the metric type to lowercase, so the internals have a single source of truth for metric type definitions. + // This is done as a convenience measure for users, so they don't have to remember the exact casing. config.Spec.Resources[i].Metrics[j].Each.Type = metric.Type(strings.ToLower(string(config.Spec.Resources[i].Metrics[j].Each.Type))) } } diff --git a/pkg/customresourcestate/config_test.go b/pkg/customresourcestate/config_test.go index 7ae11985f2..6927b3c211 100644 --- a/pkg/customresourcestate/config_test.go +++ b/pkg/customresourcestate/config_test.go @@ -32,6 +32,7 @@ var testData string func Test_Metrics_deserialization(t *testing.T) { var m Metrics assert.NoError(t, yaml.NewDecoder(strings.NewReader(testData)).Decode(&m)) + configOverrides(&m) assert.Equal(t, "active_count", m.Spec.Resources[0].Metrics[0].Name) t.Run("can create resource factory", func(t *testing.T) { diff --git a/pkg/customresourcestate/registry_factory.go b/pkg/customresourcestate/registry_factory.go index 907c786655..d5f920e466 100644 --- a/pkg/customresourcestate/registry_factory.go +++ b/pkg/customresourcestate/registry_factory.go @@ -152,7 +152,7 @@ type compiledMetric interface { // newCompiledMetric returns a compiledMetric depending on the given metric type. func newCompiledMetric(m Metric) (compiledMetric, error) { - switch metric.Type(strings.ToLower(string(m.Type))) { + switch m.Type { case metric.Gauge: if m.Gauge == nil { return nil, errors.New("expected each.gauge to not be nil") diff --git a/pkg/metric/metric.go b/pkg/metric/metric.go index 47787fe2df..d138b240a4 100644 --- a/pkg/metric/metric.go +++ b/pkg/metric/metric.go @@ -40,11 +40,6 @@ var ( // Type represents the type of the metric. See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#metric-types. type Type string -// String returns the string representation of the metric type. -func (t Type) String() string { - return string(t) -} - // Supported metric types. var ( diff --git a/pkg/metric_generator/generator.go b/pkg/metric_generator/generator.go index 9442c6554e..2a2e30221f 100644 --- a/pkg/metric_generator/generator.go +++ b/pkg/metric_generator/generator.go @@ -90,7 +90,7 @@ func (g *FamilyGenerator) generateHeader() string { header.WriteString("# TYPE ") header.WriteString(g.Name) header.WriteByte(' ') - header.WriteString(g.Type.String()) + header.WriteString(string(g.Type)) return header.String() } diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index ff446e31ae..960f350eb1 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -92,12 +92,8 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite var lastHeader string for _, writer := range writers { if len(writer.stores) > 0 { - for i, header := range writer.stores[0].headers { - - // Nothing to sanitize. - if len(header) == 0 { - continue - } + for i := 0; i < len(writer.stores[0].headers); { + header := writer.stores[0].headers[i] // Removes duplicate headers from the given MetricsWriterList for the same family (generated through CRS). // These are expected to be consecutive since G** resolution generates groups of similar metrics with same headers before moving onto the next G** spec in the CRS configuration. @@ -106,8 +102,8 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite // If the requested content type was proto-based, replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' protobuf machinery. if strings.HasPrefix(contentType, expfmt.ProtoType) { - infoTypeString := metric.Info.String() - stateSetTypeString := metric.StateSet.String() + infoTypeString := string(metric.Info) + stateSetTypeString := string(metric.StateSet) if strings.HasSuffix(header, infoTypeString) { header = header[:len(header)-len(infoTypeString)] + string(metric.Gauge) writer.stores[0].headers[i] = header @@ -121,22 +117,17 @@ func SanitizeHeaders(contentType string, writers MetricsWriterList) MetricsWrite // Nullify duplicate headers after the sanitization to not miss out on any new candidates. if header == lastHeader { - writer.stores[0].headers[i] = "" + writer.stores[0].headers = append(writer.stores[0].headers[:i], writer.stores[0].headers[i+1:]...) + + // Do not increment the index, as the next header is now at the current index. + continue } // Update the last header. lastHeader = header - } - } - } - // Remove all empty headers. - for _, writer := range writers { - if len(writer.stores) > 0 { - for i := len(writer.stores[0].headers) - 1; i >= 0; i-- { - if writer.stores[0].headers[i] == "" { - writer.stores[0].headers = append(writer.stores[0].headers[:i], writer.stores[0].headers[i+1:]...) - } + // Move to the next header. + i++ } } } diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go index 4f2ddb530b..0b61a16d63 100644 --- a/pkg/metricshandler/metrics_handler.go +++ b/pkg/metricshandler/metrics_handler.go @@ -178,8 +178,8 @@ func (m *MetricsHandler) Run(ctx context.Context) error { return ctx.Err() } -// ServeHTTP implements the http.Handler interface. It writes all generated -// metrics to the response body. +// ServeHTTP implements the http.Handler interface. It writes all generated metrics to the response body. +// Note that all operations defined within this procedure are performed at every request. func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { m.mtx.RLock() defer m.mtx.RUnlock() From 8e7b38d516296dcf40fbc7cf23366ee99f16acd6 Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Tue, 26 Mar 2024 23:47:53 +0530 Subject: [PATCH 09/10] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metrics_store/metrics_writer.go | 2 +- pkg/metricshandler/metrics_handler.go | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 960f350eb1..0e263e4654 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/pkg/metrics_store/metrics_writer.go @@ -100,7 +100,7 @@ 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, replace "info" and "statesets" with "gauge", as they are not recognized by Prometheus' protobuf machinery. + // 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) { infoTypeString := string(metric.Info) stateSetTypeString := string(metric.StateSet) diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go index 0b61a16d63..29e7c88e89 100644 --- a/pkg/metricshandler/metrics_handler.go +++ b/pkg/metricshandler/metrics_handler.go @@ -187,10 +187,9 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { var writer io.Writer = w contentType := expfmt.NegotiateIncludingOpenMetrics(r.Header) - gotContentType := contentType - // We do not support protobuf at the moment. Fall back to FmtText if the negotiated exposition format is not FmtOpenMetrics See: https://github.com/kubernetes/kube-state-metrics/issues/2022 - if contentType != expfmt.FmtOpenMetrics_1_0_0 && contentType != expfmt.FmtOpenMetrics_0_0_1 { + // Assume text/plain if no Content-Type header is set. + if contentType == expfmt.FmtUnknown { contentType = expfmt.FmtText } resHeader.Set("Content-Type", string(contentType)) @@ -209,7 +208,7 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - m.metricsWriters = metricsstore.SanitizeHeaders(string(gotContentType), m.metricsWriters) + m.metricsWriters = metricsstore.SanitizeHeaders(string(contentType), m.metricsWriters) for _, w := range m.metricsWriters { err := w.WriteAll(writer) if err != nil { From 2a06c450615c4f4a908df043a54eba9f54cb459d Mon Sep 17 00:00:00 2001 From: Pranshu Srivastava Date: Wed, 27 Mar 2024 23:53:23 +0530 Subject: [PATCH 10/10] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fix: fallback to `gauge` for `protofmt`-based negotiations --- pkg/metricshandler/metrics_handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/metricshandler/metrics_handler.go b/pkg/metricshandler/metrics_handler.go index 29e7c88e89..8cee50b4f5 100644 --- a/pkg/metricshandler/metrics_handler.go +++ b/pkg/metricshandler/metrics_handler.go @@ -188,8 +188,8 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { contentType := expfmt.NegotiateIncludingOpenMetrics(r.Header) - // Assume text/plain if no Content-Type header is set. - if contentType == expfmt.FmtUnknown { + // We do not support protobuf at the moment. Fall back to FmtText if the negotiated exposition format is not FmtOpenMetrics See: https://github.com/kubernetes/kube-state-metrics/issues/2022. + if contentType != expfmt.FmtOpenMetrics_1_0_0 && contentType != expfmt.FmtOpenMetrics_0_0_1 { contentType = expfmt.FmtText } resHeader.Set("Content-Type", string(contentType))