Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration for Prometheus to limit cardinality #1887

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

patrickbrophy
Copy link
Collaborator

This PR addresses issue #1311. This PR uses the default values as described in this design doc. This PR adds the ability to configure the label name and value length limits, the number of labels per time series, and the scrape limit. More information about these parameters can be found here

@patrickbrophy patrickbrophy added the enhancement New feature or request label Jan 13, 2025
@patrickbrophy patrickbrophy added this to the v7.13.0 milestone Jan 13, 2025
@patrickbrophy patrickbrophy linked an issue Jan 13, 2025 that may be closed by this pull request
docs/parameters.yaml Show resolved Hide resolved
docs/parameters.yaml Show resolved Hide resolved
web_ui/prometheus.go Outdated Show resolved Hide resolved
@@ -396,6 +403,13 @@ func ConfigureEmbeddedPrometheus(ctx context.Context, engine *gin.Engine) error
}},
},
}

// Cardinality limits for Prometheus, configured via Pelican configuration file
scrapeConfig.LabelLimit = uint(param.Monitoring_LabelLimit.GetInt())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't spent much time with this code in general. Why does it become necessary to set these scrape limits in three different places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a hard time wrapping my head around it as well, but I figured if we set it to often it too often it wouldn't be an issue.

Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small touchup, and then I think we're good to go (pending some failing tests).

web_ui/prometheus.go Outdated Show resolved Hide resolved
web_ui/prometheus.go Outdated Show resolved Hide resolved
@jhiemstrawisc jhiemstrawisc merged commit 55d8718 into PelicanPlatform:main Jan 22, 2025
22 checks passed
@patrickbrophy patrickbrophy requested review from jhiemstrawisc and removed request for jhiemstrawisc January 22, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safety net for director Prometheus instance
2 participants