feat: support metrics scrape #2344

jiuker · 2024-10-18T03:00:11Z

feat: support metrics scrape
fix #2327
we just support /minio/v2/metrics/cluster before
now we can do more type for this
like

prometheusOperatorScrapeMetricsPath:
- /minio/v2/metrics/cluster
- /minio/metrics/v3/api
- /minio/v2/metrics/bucket

default is /minio/v2/metrics/cluster

harshavardhana · 2024-10-18T03:02:45Z

We should add a way to scrape v3 metrics as well

cesnietor · 2024-10-28T17:02:25Z

@jiuker is there a way we can create tests for this? since this is a new feature? like an integration test? Thanks.

resources/base/crds/minio.min.io_tenants.yaml

pkg/controller/prometheus.go

jiuker · 2025-02-05T09:10:27Z

Will take a look

allanrogerr · 2025-03-10T10:02:43Z

@jiuker PTAL - pkg:golang/golang.org/x/[email protected] vulnerability

allanrogerr · 2025-03-10T12:19:12Z

pkg/controller/prometheus.go

 			}
 		}
-		if !hasScrapeConfig {
+		exceptedScrapeConfigs = append(exceptedScrapeConfigs, promCfg.ScrapeConfigs...)


The only way to remove scrapeConfigs is to set prometheusOperator to false, then re-initialize the array. This does not seem correct.

Yesh. prometheusOperator: true with this prometheusOperatorScrapeMetricsPath:[] is means default /minio/v2/metrics/cluster like before.
I think we should compatible that user don't set that before. Any advice ? @allanrogerr ?

I guess you mean expectedScrapeConfigs instead of exceptedScrapeConfigs? Or did you mean acceptedScapeConfigs???

allanrogerr · 2025-03-10T12:21:41Z

pkg/resources/configmaps/prometheus.go

+	}
+
+	for index, scrape := range t.Spec.PrometheusOperatorScrapeMetricsPath {
+		promConfig.ScrapeConfigs = append(promConfig.ScrapeConfigs, ScrapeConfig{


Consider the following tenant yaml section; note that v3 cluster metrics is scraped using /minio/v3/metrics/cluster/ (see https://github.com/minio/minio/blob/master/docs/metrics/v3.md):

prometheusOperator: true prometheusOperatorScrapeMetricsPath: - /minio/v2/metrics/bucket - /minio/v2/metrics/cluster - /minio/v2/metrics/node - /minio/v2/metrics/resource - /minio/v3/metrics/cluster/

v3 metrics scraping is failing with server returned HTTP status 403 Forbidden. Please try and check this.

Yesh. Should be like /minio/metrics/v3/*, but that can be set with this strings slice field.

ramondeklein

I'm no expert on Prometheus metrics and MinIO, but I think this code doesn't work correctly. Please make these changes to AIStor operator and implement proper integration tests.

ramondeklein · 2025-03-10T15:10:15Z

pkg/apis/minio.min.io/v2/types.go

+	// The name of the Prometheus instance to scrape metrics from.
+	//
+	// +optional
+	PrometheusOperatorScrapeMetricsPath []string `json:"prometheusOperatorScrapeMetricsPath,omitempty"`


If you can specify multiple values, then this should probably be:

Suggested change

PrometheusOperatorScrapeMetricsPath []string `json:"prometheusOperatorScrapeMetricsPath,omitempty"`

PrometheusOperatorScrapeMetricsPaths []string `json:"prometheusOperatorScrapeMetricsPaths,omitempty"`

ramondeklein · 2025-03-10T15:12:02Z

pkg/apis/minio.min.io/v2/types.go

@@ -278,6 +278,12 @@ type TenantSpec struct {
 	PrometheusOperator bool `json:"prometheusOperator,omitempty"`
 	// *Optional* +
 	//
+	// The name of the Prometheus instance to scrape metrics from.
+	//
+	// +optional


Can we add some information about the default (or fallback) value?

ramondeklein · 2025-03-11T09:12:32Z

pkg/controller/prometheus.go

+		} else {
+			for i := range scrapeConfigs {
+				if scrapeConfigs[i].JobName != exceptedScrapeConfigs[i].JobName ||
+					scrapeConfigs[i].MetricsPath != exceptedScrapeConfigs[i].MetricsPath ||


Please add some comment that the generation of the scrape configuration always creates a new bearer token, so it shouldn't be included in this check, because it will always result in it being not equal.

How do we make sure the bearer token is updated when the tenant gets assigned other credentials (different access/secret key). This will result in another bearer token (but nothing else), so it will decide that the scape configuration hasn't changed, but Prometheus won't be able to access MinIO with the old token.

ramondeklein · 2025-03-11T09:14:41Z

pkg/controller/prometheus.go

@@ -161,20 +163,34 @@ func (c *Controller) checkAndCreatePrometheusAddlConfig(ctx context.Context, ten
 			return err
 		}
 	} else {
-		var scrapeConfigs []configmaps.ScrapeConfig
+		var scrapeConfigs, exceptedScrapeConfigs []configmaps.ScrapeConfig


Can we change line 146 to include the namespace/tenant that is getting the Prometheus scrape configuration?

pkg/controller/prometheus.go

ramondeklein · 2025-03-11T09:43:24Z

pkg/controller/prometheus.go

 			}
 		}
-		if !hasScrapeConfig {
+		exceptedScrapeConfigs = append(exceptedScrapeConfigs, promCfg.ScrapeConfigs...)


I guess you mean expectedScrapeConfigs instead of exceptedScrapeConfigs? Or did you mean acceptedScapeConfigs???

refactor

UT

ut

lint

ramondeklein · 2025-03-13T10:56:41Z

pkg/apis/minio.min.io/v2/helper.go

+// GetAccessKeyFromBearerToken parses the BearerToken with secretKey to extract accessKey
+func GetAccessKeyFromBearerToken(bearerToken string, secretKey string) (string, error) {
+	claims := &jwt.StandardClaims{}
+	token, err := jwt.ParseWithClaims(bearerToken, claims, func(token *jwt.Token) (interface{}, error) {


Please address linter issue:

Suggested change

token, err := jwt.ParseWithClaims(bearerToken, claims, func(token *jwt.Token) (interface{}, error) {

token, err := jwt.ParseWithClaims(bearerToken, claims, func(_ *jwt.Token) (interface{}, error) {

ramondeklein · 2025-03-13T11:47:59Z

pkg/controller/prometheus.go

@@ -224,27 +247,20 @@ func (c *Controller) deletePrometheusAddlConfig(ctx context.Context, tenant *min
 		return err
 	}

-	var scrapeConfigs []configmaps.ScrapeConfig
+	var scrapeConfigs, exceptedScrapeConfigs []configmaps.ScrapeConfig


I think it's a typo:

Suggested change

var scrapeConfigs, exceptedScrapeConfigs []configmaps.ScrapeConfig

var scrapeConfigs, expectedScrapeConfigs []configmaps.ScrapeConfig

should be fine now

ramondeklein · 2025-03-13T11:52:29Z

pkg/controller/prometheus.go

+				}
+				accKey, err := miniov2.GetAccessKeyFromBearerToken(scrapeConfigs[i].BearerToken, secretKey)
+				if err != nil {
+					klog.Errorf("Failed to get access key from bearer token: %v", err)


When secretKey is changed, then it will not be able to decrypt the existing bearer-token using the new secret key. The function will return an error. That shouldn't be logged, but is expected, but it should set updateScrapeConfig.

ignored it now

pkg/controller/prometheus.go

apply suggestion

jiuker requested review from pjuarezd, ramondeklein, harshavardhana, cniackz, allanrogerr and dvaldivia October 18, 2024 03:01

jiuker requested a review from shtripat October 18, 2024 07:43

jiuker self-assigned this Oct 21, 2024

pjuarezd force-pushed the feat-support-metrics-scrope branch from b38e1b6 to 982f5d4 Compare December 9, 2024 19:51

pjuarezd previously approved these changes Dec 9, 2024

View reviewed changes

allanrogerr reviewed Dec 9, 2024

View reviewed changes

resources/base/crds/minio.min.io_tenants.yaml Outdated Show resolved Hide resolved

allanrogerr reviewed Dec 10, 2024

View reviewed changes

pkg/controller/prometheus.go Show resolved Hide resolved

jiuker dismissed pjuarezd’s stale review via 2581b25 February 6, 2025 02:16

jiuker force-pushed the feat-support-metrics-scrope branch from 2c7c9e0 to 40012a7 Compare February 6, 2025 02:49

redo compare

732b95c

jiuker force-pushed the feat-support-metrics-scrope branch from 40012a7 to 732b95c Compare February 6, 2025 03:52

jiuker requested review from pjuarezd and allanrogerr February 6, 2025 03:55

Merge branch 'minio:master' into feat-support-metrics-scrope

ebba636

allanrogerr requested changes Mar 10, 2025

View reviewed changes

jiuker requested a review from allanrogerr March 11, 2025 07:36

ramondeklein requested changes Mar 11, 2025

View reviewed changes

jiuker added 2 commits March 12, 2025 13:16

refactor

957ad32

refactor

UT

f583084

UT

jiuker requested a review from ramondeklein March 12, 2025 05:25

ut

24fe5b2

ut

jiuker and others added 2 commits March 12, 2025 13:33

lint

771e8bd

lint

Merge branch 'minio:master' into feat-support-metrics-scrope

7dcb96a

ramondeklein requested changes Mar 13, 2025

View reviewed changes

apply suggestion

5a6e965

apply suggestion

jiuker requested a review from ramondeklein March 14, 2025 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support metrics scrape #2344

feat: support metrics scrape #2344

jiuker commented Oct 18, 2024 •

edited

Loading

harshavardhana commented Oct 18, 2024

cesnietor commented Oct 28, 2024

jiuker commented Feb 5, 2025

allanrogerr commented Mar 10, 2025

allanrogerr Mar 10, 2025

jiuker Mar 11, 2025 •

edited

Loading

ramondeklein Mar 11, 2025

allanrogerr Mar 10, 2025

jiuker Mar 11, 2025 •

edited

Loading

ramondeklein left a comment

ramondeklein Mar 10, 2025

ramondeklein Mar 10, 2025

ramondeklein Mar 11, 2025

ramondeklein Mar 11, 2025

ramondeklein Mar 11, 2025

ramondeklein Mar 13, 2025

ramondeklein Mar 13, 2025

jiuker Mar 14, 2025

ramondeklein Mar 13, 2025

jiuker Mar 14, 2025

	PrometheusOperatorScrapeMetricsPath []string `json:"prometheusOperatorScrapeMetricsPath,omitempty"`
	PrometheusOperatorScrapeMetricsPaths []string `json:"prometheusOperatorScrapeMetricsPaths,omitempty"`

	token, err := jwt.ParseWithClaims(bearerToken, claims, func(token *jwt.Token) (interface{}, error) {
	token, err := jwt.ParseWithClaims(bearerToken, claims, func(_ *jwt.Token) (interface{}, error) {

	var scrapeConfigs, exceptedScrapeConfigs []configmaps.ScrapeConfig
	var scrapeConfigs, expectedScrapeConfigs []configmaps.ScrapeConfig

feat: support metrics scrape #2344

Are you sure you want to change the base?

feat: support metrics scrape #2344

Conversation

jiuker commented Oct 18, 2024 • edited Loading

harshavardhana commented Oct 18, 2024

cesnietor commented Oct 28, 2024

jiuker commented Feb 5, 2025

allanrogerr commented Mar 10, 2025

Choose a reason for hiding this comment

jiuker Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiuker Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

ramondeklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiuker commented Oct 18, 2024 •

edited

Loading

jiuker Mar 11, 2025 •

edited

Loading

jiuker Mar 11, 2025 •

edited

Loading