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

WIP: Update vendor github.com/prometheus/common/... #373

Closed
wants to merge 4 commits into from

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Oct 15, 2018

Follow-up to #372

Signed-off-by: Ben Kochie superq@gmail.com

Follow-up to #372

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ requested a review from beorn7 October 15, 2018 16:53
* github.com/beorn7/perks/quantile
* github.com/golang/protobuf/proto@v1.2.0
* github.com/prometheus/procfs

Signed-off-by: Ben Kochie <superq@gmail.com>
@beorn7
Copy link
Member

beorn7 commented Oct 15, 2018

Oooh, something got removed from common/config...

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ
Copy link
Member Author

SuperQ commented Oct 15, 2018

@beorn7 Yea, this one was way out of date. Looks like the use was changed slightly. Pushed a fix.

Signed-off-by: Ben Kochie <superq@gmail.com>
@brian-brazil
Copy link
Contributor

That would enable keepalives for HTTP, which we don't really want for blackbox.

@beorn7
Copy link
Member

beorn7 commented Oct 15, 2018

Is that the story where somebody decided keepalive is always good?

@brian-brazil
Copy link
Contributor

No, different thing. Someone was doing some other work which removed the option to disable it, and I hadn't gotten around to fixing it all yet.

@beorn7
Copy link
Member

beorn7 commented Oct 15, 2018

Alright. I'd say for now it's fine to not update the prometheus/common vendoring as the metric response size of the blackbox exporter is usually small.

@SuperQ SuperQ changed the title Update vendor github.com/prometheus/common/... WIP: Update vendor github.com/prometheus/common/... Oct 15, 2018
@beorn7
Copy link
Member

beorn7 commented Oct 16, 2018

Hmm, following prometheus/client_golang#366 , it seems my assumption was wrong and a blackbox exporter might run quite a bit of encoding load.

So I guess we should update at least the vendoring of the expfmt package (which could leave out the HTTP config stuff, even if it is ugly to vendor different commits from the same repo) to pull in the text format creation improvement. We should also consider to allow users to disable gzip compression (client_golang allows this via HandlerOpts, but it has to be exposed by a command line flag for the blackbox_exporter).

@beorn7
Copy link
Member

beorn7 commented Nov 15, 2018

How about my suggestion above?

@brian-brazil
Copy link
Contributor

Partial vendoring sounds okay to me.

@beorn7
Copy link
Member

beorn7 commented Apr 3, 2019

I guess with Go modules (as they are now used in blackbox_exporter), the partial vendoring doesn't work anymore. I guess it makes most sense to close this PR and do a normal Go modules update once common is fixed.

@beorn7 beorn7 closed this Apr 3, 2019
@SuperQ SuperQ deleted the bjk/prom_common branch March 9, 2021 10:37
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