Skip to content

Conversation

TheEdgeOfRage
Copy link

title: Add sampleLimit to ServiceMonitor
chore: Support optionally setting the prometheus scrape sampleLimit parameter on the ServiceMonitor

How was this change tested?

Running helm template with various values

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@TheEdgeOfRage TheEdgeOfRage requested a review from a team as a code owner October 3, 2025 00:22
@TheEdgeOfRage TheEdgeOfRage requested a review from bwagner5 October 3, 2025 00:22
Copy link

netlify bot commented Oct 3, 2025

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 84d7418
🔍 Latest deploy log https://app.netlify.com/projects/karpenter-docs-prod/deploys/68f2a39033f4e100082acbb3

Copy link
Contributor

@DerekFrank DerekFrank left a comment

Choose a reason for hiding this comment

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

Could you add more details about your usecase? I understand the general need for limiting the samples, but is your goal to up the default or to limit it because you're hitting scrape timeouts?

@TheEdgeOfRage
Copy link
Author

We have a global sample limit configured on prometheus to 10k, which causes it to fail to scrape karpenter, since it exposes a lot more than that (around 70k IIRC). We still want to have the default scrape limit on prometheus to protect against too high metric cardinality on our services, but allow specific services to override the limit when needed, like here

Copy link
Contributor

github-actions bot commented Oct 15, 2025

Preview deployment ready!

Preview URL: https://pr-8557.d18coufmbnnaag.amplifyapp.com

Built from commit 84d7418763eb5af9464d1c1e1a18712d07f452e9

@coveralls
Copy link

coveralls commented Oct 15, 2025

Pull Request Test Coverage Report for Build 18603766286

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 67.517%

Totals Coverage Status
Change from base Build 18576086927: 0.0%
Covered Lines: 7782
Relevant Lines: 11526

💛 - Coveralls

@DerekFrank
Copy link
Contributor

I think that usecase makes sense. Let me double check with some other team members to make sure I'm not missing anything and we can get this merged

Copy link
Contributor

@DerekFrank DerekFrank left a comment

Choose a reason for hiding this comment

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

Alright, after discussion with the team looks like we're good to go, just update the branch to be up to date and I'll approve!

@TheEdgeOfRage
Copy link
Author

Updated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants