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

*: move to go 1.11 #4626

Merged
merged 2 commits into from Oct 16, 2018
Merged

*: move to go 1.11 #4626

merged 2 commits into from Oct 16, 2018

Conversation

simonpasquier
Copy link
Member

No description provided.

@gouthamve
Copy link
Member

While I'm not against it, it is worth noting that this is going to break a lot of kubernetes users badly (including me). See: golang/go#27546

@grobie
Copy link
Member

grobie commented Sep 18, 2018

@gouthamve Why? Prometheus doesn't use the native go DNS client.

@simonpasquier
Copy link
Member Author

/benchmark pr

@gouthamve
Copy link
Member

@grobie I didn't know that. We upgraded Cortex and that broke some stuff, best test it out though. I'll deploy this one soon.

@simonpasquier
Copy link
Member Author

Yeah, I would definitely be cautious with this change: DNS + Go is a finicky topic.

@simonpasquier
Copy link
Member Author

/benchmark

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@simonpasquier: Benchmarking is restricted to org members.

In response to this:

/benchmark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gouthamve
Copy link
Member

/benchmark

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@gouthamve: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4626 and master

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

/benchmark cancel

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@simonpasquier: Looks like start-benchmark job is already running on this PR. Will start cancel-benchmark job once ongoing job is completed

In response to this:

/benchmark cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gouthamve
Copy link
Member

/benchmark cancel

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@gouthamve: Looks like a job is already lined up for this PR.
Please try again once all pending jobs have finished 😃

In response to this:

/benchmark cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

/benchmark

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@simonpasquier: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4626 and master

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

/benchmark cancel

@prombot prombot removed the benchmark label Sep 18, 2018
@simonpasquier
Copy link
Member Author

/benchmark pr

@sipian
Copy link
Contributor

sipian commented Sep 18, 2018

@simonpasquier Acc to https://github.com/prometheus/prombench#how-to-trigger-tests-on-github

A Prometheus maintainer can comment as follows to benchmark a PR:
/benchmark <-- This will benchmark PR with the master branch.
/benchmark master
/benchmark 2.4.0 <-- Any release version can be added here. Don't prepend v to the release version.
To cancel benchmarking, a mantainer should comment /benchmark cancel.

Benchmarking will always include the PR.
You need to specify the release version with which you want to compare the PR (master | previous-release)

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@sipian: Benchmarking is restricted to org members.

In response to this:

@simonpasquier Acc to https://github.com/prometheus/prombench#how-to-trigger-tests-on-github

A Prometheus maintainer can comment as follows to benchmark a PR:
/benchmark <-- This will benchmark PR with the master branch.
/benchmark master
/benchmark 2.4.0 <-- Any release version can be added here. Don't prepend v to the release version.
To cancel benchmarking, a mantainer should comment /benchmark cancel.

Benchmarking will always include the PR.
You need to specify the release version with which you want to compare the PR (master | previous-release)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

@sipian yep but there are some places that still reference /benchmark pr. I'll send a PR to fix some of the documentation on prombench.

@sipian
Copy link
Contributor

sipian commented Sep 18, 2018

Aah
Thanks for pointing this out.
[EDIT]
prometheus/test-infra#155

@simonpasquier
Copy link
Member Author

/benchmark

@prombot
Copy link
Contributor

prombot commented Sep 18, 2018

@simonpasquier: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4626 and master

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simonpasquier
Copy link
Member Author

Regarding the DNS service discovery, @grobie noted correctly that it uses https://github.com/miekg/dns instead of the native Go client so it should be immune to golang/go#27546. I did a test with a domain query returning a bunch of SRV records and it worked fine. Again not saying that it is sufficient and additional testing would be much appreciated.

@krasi-georgiev
Copy link
Contributor

/benchmark

@prombot
Copy link
Contributor

prombot commented Oct 8, 2018

@krasi-georgiev: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4626 and master

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark

If you have questions or suggestions related to my behavior, please file an issue against the prometheus/prombench repository.

@simonpasquier
Copy link
Member Author

/benchmark cancel

@prombot prombot removed the benchmark label Oct 8, 2018
@simonpasquier
Copy link
Member Author

/benchmark 2.4.2

@prombot
Copy link
Contributor

prombot commented Oct 9, 2018

@simonpasquier: Welcome to Prometheus Benchmarking Tool.

The two prometheus versions that will be compared are pr-4626 and v2.4.2

The logs can be viewed at the links provided in the GitHub check blocks at the end of this conversation

After successfull deployment, the benchmarking metrics can be viewed at :

To stop the benchmark process comment /benchmark cancel .

In response to this:

/benchmark 2.4.2

If you have questions or suggestions related to my behavior, please file an issue against the prometheus/prombench repository.

@simonpasquier
Copy link
Member Author

no big differences between the v2.4.2 (go1.10.3) and go1.11 builds apart a very small decrease of CPU usage with 1.11.

/benchmark cancel

@prombot prombot removed the benchmark label Oct 9, 2018
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier
Copy link
Member Author

As mentioned on IRC, Prometheus v2.4.3 is already built with go1.11.1 as promu had switched its default Go version to 1.11 before v2.4.3 was released. To prevent this from happening again, this PR also sets the Go version in .promu.yml.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@hoffie
Copy link
Contributor

hoffie commented Oct 10, 2018

@simonpasquier
There are reports of go1.11 allocating more virtual memory than earlier versions: golang/go#28114

Not sure if relevant for Prometheus (did not try). It could be just cosmetic. Mentioning just in case any such reports come in. Maybe v2.4.3 having been released without feedback regarding this is already a positive sign that this is a non-issue.

@simonpasquier
Copy link
Member Author

@simonpasquier
Copy link
Member Author

As noted above, v2.4.3 has already been built with go1.11.1 so I'd say that this PR is ok to go.

@gouthamve @brian-brazil WDYT?

@brian-brazil
Copy link
Contributor

👍

Copy link
Member

@grobie grobie left a comment

Choose a reason for hiding this comment

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

LGTM

@simonpasquier simonpasquier merged commit c4a6acf into prometheus:master Oct 16, 2018
@simonpasquier simonpasquier deleted the go-1.11 branch October 16, 2018 07:44
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.

None yet

8 participants