From 4c8562bceca62b591f0259324239afabfdbda4be Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Tue, 30 Jul 2024 15:28:46 -0700 Subject: [PATCH] Fix v2 metrics: Send all ttfb api labels (#20191) Fix a regression in #19733 where TTFB metrics for all APIs except GetObject were removed in v2 and v3 metrics. This causes breakage for existing v2 metrics users. Instead we continue to send TTFB for all APIs in V2 but only send for GetObject in V3. --- cmd/http-stats.go | 10 ++-------- cmd/metrics-v3-api.go | 8 ++++++-- cmd/metrics-v3-types.go | 23 +++++++++++++++++------ 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/cmd/http-stats.go b/cmd/http-stats.go index 3fb528c1e..077a72575 100644 --- a/cmd/http-stats.go +++ b/cmd/http-stats.go @@ -27,10 +27,6 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -const ( - apiGetObject = "GetObject" -) - // connStats - Network statistics // Count total input/output transferred bytes during // the server's life. @@ -132,7 +128,7 @@ func (bh *bucketHTTPStats) updateHTTPStats(bucket, api string, w *xhttp.Response return } - if w != nil && api == apiGetObject { + if w != nil { // Increment the prometheus http request response histogram with API, Bucket bucketHTTPRequestsDuration.With(prometheus.Labels{ "api": api, @@ -437,9 +433,7 @@ func (st *HTTPStats) updateStats(api string, w *xhttp.ResponseRecorder) { st.totalS3Requests.Inc(api) // Increment the prometheus http request response histogram with appropriate label - if api == apiGetObject { - httpRequestsDuration.With(prometheus.Labels{"api": api}).Observe(w.TimeToFirstByte.Seconds()) - } + httpRequestsDuration.With(prometheus.Labels{"api": api}).Observe(w.TimeToFirstByte.Seconds()) code := w.StatusCode diff --git a/cmd/metrics-v3-api.go b/cmd/metrics-v3-api.go index 548d72172..2f35bb3ce 100644 --- a/cmd/metrics-v3-api.go +++ b/cmd/metrics-v3-api.go @@ -19,6 +19,8 @@ package cmd import ( "context" + + "github.com/minio/minio-go/v7/pkg/set" ) const ( @@ -127,7 +129,8 @@ func loadAPIRequestsHTTPMetrics(ctx context.Context, m MetricValues, _ *metricsC // This is a `MetricsLoaderFn`. func loadAPIRequestsTTFBMetrics(ctx context.Context, m MetricValues, _ *metricsCache) error { renameLabels := map[string]string{"api": "name"} - m.SetHistogram(apiRequestsTTFBSecondsDistribution, httpRequestsDuration, renameLabels, nil, + labelsFilter := map[string]set.StringSet{"api": set.CreateStringSet("GetObject")} + m.SetHistogram(apiRequestsTTFBSecondsDistribution, httpRequestsDuration, labelsFilter, renameLabels, nil, "type", "s3") return nil } @@ -214,7 +217,8 @@ func loadBucketAPIHTTPMetrics(ctx context.Context, m MetricValues, _ *metricsCac // This is a `MetricsLoaderFn`. func loadBucketAPITTFBMetrics(ctx context.Context, m MetricValues, _ *metricsCache, buckets []string) error { renameLabels := map[string]string{"api": "name"} - m.SetHistogram(apiRequestsTTFBSecondsDistribution, bucketHTTPRequestsDuration, renameLabels, + labelsFilter := map[string]set.StringSet{"api": set.CreateStringSet("GetObject")} + m.SetHistogram(apiRequestsTTFBSecondsDistribution, bucketHTTPRequestsDuration, labelsFilter, renameLabels, buckets, "type", "s3") return nil } diff --git a/cmd/metrics-v3-types.go b/cmd/metrics-v3-types.go index f891cf947..859824ef8 100644 --- a/cmd/metrics-v3-types.go +++ b/cmd/metrics-v3-types.go @@ -23,6 +23,7 @@ import ( "strings" "sync" + "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/minio/internal/logger" "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" @@ -247,28 +248,38 @@ func (m *MetricValues) Set(name MetricName, value float64, labels ...string) { // SetHistogram - sets values for the given MetricName using the provided // histogram. // +// `filterByLabels` is a map of label names to list of allowed label values to +// filter by. Note that this filtering happens before any renaming of labels. +// // `renameLabels` is a map of label names to rename. The keys are the original // label names and the values are the new label names. // -// TODO: bucketFilter doc +// `bucketFilter` is a list of bucket values to filter. If this is non-empty, +// only metrics for the given buckets are added. // // `extraLabels` are additional labels to add to each metric. They are ordered // label name and value pairs. func (m *MetricValues) SetHistogram(name MetricName, hist *prometheus.HistogramVec, - renameLabels map[string]string, bucketFilter []string, extraLabels ...string, + filterByLabels map[string]set.StringSet, renameLabels map[string]string, bucketFilter []string, + extraLabels ...string, ) { if _, ok := m.descriptors[name]; !ok { panic(fmt.Sprintf("metric has no description: %s", name)) } dummyDesc := MetricDescription{} metricsV2 := getHistogramMetrics(hist, dummyDesc, false) +mainLoop: for _, metric := range metricsV2 { + for label, allowedValues := range filterByLabels { + if !allowedValues.Contains(metric.VariableLabels[label]) { + continue mainLoop + } + } + // If a bucket filter is provided, only add metrics for the given // buckets. - if len(bucketFilter) > 0 { - if !slices.Contains(bucketFilter, metric.VariableLabels["bucket"]) { - continue - } + if len(bucketFilter) > 0 && !slices.Contains(bucketFilter, metric.VariableLabels["bucket"]) { + continue } labels := make([]string, 0, len(metric.VariableLabels)*2)