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

Removal of "prometheus.io/scrape", etc. from kubernetes example #4484

Closed
schweikert opened this Issue Aug 10, 2018 · 10 comments

Comments

Projects
None yet
2 participants
@schweikert
Copy link

schweikert commented Aug 10, 2018

I am a bit puzzled by this commit:
03a9e7f

Is there a reference where this was discussed? Even though this is a convention and not something that needs to be done that way, I fail to see how completely removing this and replacing it with pseudo code is helping users. Even though it wasn't formalized, having a sort-of-agreed-upon way of doing annotations for prometheus in kubernetes is valuable, in my opinion.

@schweikert

This comment has been minimized.

Copy link
Author

schweikert commented Aug 10, 2018

@Bplotka : maybe you could comment?

@bwplotka

This comment has been minimized.

Copy link
Contributor

bwplotka commented Aug 10, 2018

@schweikert sure, can comment. (:

Here is the reference for the discussion and all the reasons of that change in examples: #4131

Let us know if something needs to be explained in further details.

@bwplotka

This comment has been minimized.

Copy link
Contributor

bwplotka commented Aug 10, 2018

I don't know why commit does not reference the PR ):

@bwplotka

This comment has been minimized.

Copy link
Contributor

bwplotka commented Aug 10, 2018

TL;DR removing those is helping users because it discourages to use those annotations in a literal way without understanding the implications.

@schweikert

This comment has been minimized.

Copy link
Author

schweikert commented Aug 10, 2018

Thanks for the link to that github issue. That's what I was looking for. I searched for it, but failed to find it :)

Anyway, personally I think that this is a bit of a shame. I think the old suggested way has the advantage of being very simple and I find it to be very useful for most of my use cases. I don't have often the problem of having two containers that need scraping in the same pod. Even for that use case, I was thinking that you could spin up a low-resource consumption sidecar that proxies the request and merges metrics from all the containers that you want scraped. This would have the advantage of keeping the simple use case simple, and making the complex use case possible.

@bwplotka

This comment has been minimized.

Copy link
Contributor

bwplotka commented Aug 10, 2018

I agree that you can still use annotation if your use case is simple. In fact, what we did in this PR we just tried to make it clear and explain more how to use simple annotations and how it works. We did not remove any example. We just made it clear that this is only an example and not "recommended" and "golden" way for doing this.

The main reason of the change is the fact that people were referencing those annotations as the perfect "way" and that Prometheus recommends it etc etc, while it does not recommend anything in this field - it just gives different ways to handle different use cases.

Do you think that adding more explanation as we did in this change was a wrong idea?

@schweikert

This comment has been minimized.

Copy link
Author

schweikert commented Aug 10, 2018

The change removed for example prometheus.io/scrape and replaced with example.io/should_be_scraped. I think that's a bit too drastic and means that all users that want to configure this, need also to think about how to name that annotation. Having a sort-of-standard for that simple use case was useful.

@schweikert

This comment has been minimized.

Copy link
Author

schweikert commented Aug 10, 2018

I have added a reply to this google groups thread:
https://groups.google.com/forum/#!topic/prometheus-users/ihMUWtX477Q

@schweikert

This comment has been minimized.

Copy link
Author

schweikert commented Aug 10, 2018

This issue probably can be closed, considering that the change was reviewed and agreed on. Move the discussion to prometheus-users. Thanks again Bplotka for your answer. Also, I learned today about how to find a pull request related to a certain github commit (there is actually a link to #4131 when looking at the commit: 03a9e7f)

@schweikert schweikert closed this Aug 10, 2018

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.