Skip to content

ScrapeConfig CRD: Proposal For Extending The CRD Surface #6634

Merged
simonpasquier merged 5 commits intoprometheus-operator:mainfrom
mviswanathsai:proposal-scrapeconfig
Oct 11, 2024
Merged

ScrapeConfig CRD: Proposal For Extending The CRD Surface #6634
simonpasquier merged 5 commits intoprometheus-operator:mainfrom
mviswanathsai:proposal-scrapeconfig

Conversation

@mviswanathsai
Copy link
Contributor

@mviswanathsai mviswanathsai commented May 30, 2024

Description

Proposal for extending the ScrapeConfig API surface to pave the way for graduation to v1beta1.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • [ x] NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.


@mviswanathsai mviswanathsai requested a review from a team as a code owner May 30, 2024 08:14
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

This is a good start!

@mviswanathsai mviswanathsai force-pushed the proposal-scrapeconfig branch 2 times, most recently from d76af43 to 4245c80 Compare May 31, 2024 07:36
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Good start, it's great that we're already thinking about graduation tasks even though the GSoC proposal was only about adding new SDs to the scrape config! I really like that you're taking one step further (@xiu will be proud to see this 😛)

There are opportunities to improve the proposal, I got the impression that you wrote the proposal with your hands tied by your GSoC project and it turned the proposal too broad. Is this proposal about ScrapeConfig graduation or is it about implementing missing fields? We already know that we want to eventually implement all SDs and missing fields, there's no confusion about it and implementing them is straightforward.

Graduating the CRD is also something that we already know we want, but the path to it is not straightforward, therefore, writing a proposal is super beneficial to find consensus in our community.

My advice here would be to refactor the proposal, focusing on how the graduation path looks like. If implementing a set of SDs is a requirement, let's be clear about which ones and why. It's ok to not have SDs as requirement, in this case, we can just say so :) It would be nice to clarify which tasks, e.g. testing, SD implementation, refactoring Monitor CRDs, are planned to each graduation phase

@mviswanathsai mviswanathsai force-pushed the proposal-scrapeconfig branch 2 times, most recently from f6b0bf5 to e313307 Compare June 6, 2024 11:33
Copy link
Contributor

@slashpai slashpai left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, a few nits


#### Requirements for Graduation

I feel that we can graduate the CRD to beta when the following milestones are all achieved:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
I feel that we can graduate the CRD to beta when the following milestones are all achieved:
We propose to graduate the CRD to beta when the following milestones are all achieved:

@mviswanathsai mviswanathsai force-pushed the proposal-scrapeconfig branch from 360e829 to 1b8c6a1 Compare July 3, 2024 12:10
@mviswanathsai
Copy link
Contributor Author

Hey! any further comments?

Copy link
Contributor

@simonpasquier simonpasquier 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 also address the other comments? Thanks!


## Non-Goals

- Implementing all 28 Service Discovery configurations immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Implementing all 28 Service Discovery configurations immediately.
- Implementing the 28 Service Discovery configurations currently supported by Prometheus.

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

I've reviewed the tiers list and made some suggestions. Could you also review @slashpai comments and replace "I ..." by "We ..."?

@mviswanathsai
Copy link
Contributor Author

PTAL @simonpasquier @slashpai

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

Almost good! I'll commit my suggestions and merge. If I was wrong, please push an update.

simonpasquier
simonpasquier previously approved these changes Oct 10, 2024
@simonpasquier
Copy link
Contributor

@mviswanathsai my changes don't pass the doc linter. Would you mind fixing the format and I'll merge :)

@simonpasquier simonpasquier enabled auto-merge (squash) October 11, 2024 15:21
@simonpasquier simonpasquier merged commit 455683e into prometheus-operator:main Oct 11, 2024
@simonpasquier
Copy link
Contributor

🥳

@mviswanathsai
Copy link
Contributor Author

Woah, finally!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants