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

Merge release-1.12 branch back into main #1079

Merged
merged 14 commits into from Jul 6, 2022
Merged

Merge release-1.12 branch back into main #1079

merged 14 commits into from Jul 6, 2022

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jul 6, 2022

So that all the commits in the release branch are also in main.

(That's what we do in prometheus/prometheus. I got very confused when I noticed that the commit released as 1.12.2 is not in the main branch. It is unfortunate that so many commits get duplicated without a change in the code, but that's what you get when you cherrypick/rebase.)

kakkoyun and others added 14 commits Jan 18, 2022
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
A previous PR made it so that the Go 1.17 collector locked only around
uses of rmSampleBuf, but really that means that Metric values may be
sent over the channel containing some values from future metrics.Read
calls. While generally-speaking this isn't a problem, we lose any
consistency guarantees provided by the runtime/metrics package.

Also, that optimization to not just lock around all of Collect was
premature. Truthfully, Collect is called relatively infrequently, and
its critical path is fairly fast (10s of µs). To prove it, this change
also adds a benchmark.

name            old time/op  new time/op  delta
GoCollector-16  43.7µs ± 2%  43.2µs ± 2%   ~     (p=0.190 n=9+9)

Note that because the benchmark is single-threaded it actually looks
like it might be getting *slightly* faster, because all those Collect
calls for the Metrics are direct calls instead of interface calls.

Signed-off-by: Michael Anthony Knyszek <mknyszek@google.com>
Replace `io.ReadAll` with `bytes.Buffer.ReadFrom`.
Both need to resize a buffer until they have finished reading;
the former increases by 1.25x each time while the latter uses 2x.

Also added a benchmark to demonstrate the benefit:
name             old time/op    new time/op    delta
Client/4KB-8       35.9µs ± 4%    35.3µs ± 3%     ~     (p=0.310 n=5+5)
Client/50KB-8      83.1µs ± 8%    69.5µs ± 1%  -16.37%  (p=0.008 n=5+5)
Client/1000KB-8     891µs ± 6%     750µs ± 0%  -15.83%  (p=0.016 n=5+4)
Client/2000KB-8    1.74ms ± 2%    1.35ms ± 1%  -22.72%  (p=0.008 n=5+5)

name             old alloc/op   new alloc/op   delta
Client/4KB-8       20.2kB ± 0%    20.4kB ± 0%   +1.26%  (p=0.008 n=5+5)
Client/50KB-8       218kB ± 0%     136kB ± 0%  -37.65%  (p=0.008 n=5+5)
Client/1000KB-8    5.88MB ± 0%    2.11MB ± 0%  -64.10%  (p=0.008 n=5+5)
Client/2000KB-8    11.7MB ± 0%     4.2MB ± 0%  -63.93%  (p=0.008 n=5+5)

name             old allocs/op  new allocs/op  delta
Client/4KB-8         75.0 ± 0%      72.0 ± 0%   -4.00%  (p=0.008 n=5+5)
Client/50KB-8         109 ± 0%        98 ± 0%  -10.09%  (p=0.079 n=4+5)
Client/1000KB-8       617 ± 0%       593 ± 0%   -3.89%  (p=0.008 n=5+5)
Client/2000KB-8     1.13k ± 0%     1.09k ± 0%   -3.27%  (p=0.008 n=5+5)

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
The Go runtime/metrics package currently exports extremely granular
histograms. Exponentially bucket any histogram with unit "seconds"
or "bytes" instead to dramatically reduce the number of buckets, and
thus the number of metrics.

This change also adds a test to check for expected cardinality to
prevent cardinality surprises in the future.

Signed-off-by: Michael Anthony Knyszek <mknyszek@google.com>
* Cut v1.12.1

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Apply review suggestions

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Update `examples/random/main.go`:
  `prometheus.NewBuildInfoCollector` is deprecated. Use `collectors.NewBuildInfoCollector` instead.

Signed-off-by: alissa-tung <alissa-tung@outlook.com>
* Renamed files.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

* gocollector: Added options to Go Collector for diffetent collections.

Fixes #983

Also:

* fixed TestMemStatsEquivalence, it was noop before (:
* Removed gc_cpu_fraction metric completely, since it's not working completely for Go1.17+

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
… metrics by default. (#1033)

Fixes #967

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
…trics (#1048)

* Fix convention violating names for generated collector metrics

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Add new Go collector example

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Remove -Inf buckets from go collector histograms

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update prometheus/collectors/go_collector_latest_test.go

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Simplify

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Cut v1.12.2

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Apply suggestions

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Update CHANGELOG.md

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
@beorn7 beorn7 requested review from bwplotka and kakkoyun Jul 6, 2022
@beorn7
Copy link
Member Author

beorn7 commented Jul 6, 2022

It also confuses Go Modules…

@kakkoyun kakkoyun merged commit c6a634f into main Jul 6, 2022
7 checks passed
@kakkoyun kakkoyun deleted the beorn7/release branch Jul 6, 2022
@beorn7
Copy link
Member Author

beorn7 commented Jul 6, 2022

@kakkoyun you didn't properly merge this, you did "squash and merge". Which created new commits all over again.

I'll do it again, creating a new PR, but this time you really have to merge.

@kakkoyun
Copy link
Member

kakkoyun commented Jul 6, 2022

@kakkoyun you didn't properly merge this, you did "squash and merge". Which created new commits all over again.

I'll do it again, creating a new PR, but this time you really have to merge.

@beorn7 Sorry for that. @bwplotka and I decided to only have the squash merge. And it's the only option now.
And I don't have the necessary access to it. Could you please open that first?

We can also revert the previous merge to keep things clean.

Again sorry for the inconvenience.

@beorn7
Copy link
Member Author

beorn7 commented Jul 6, 2022

Reverting anything in the main branch is even worse than squashing.

It's your repo, so you can define the release procedure. I was simply assuming that you would try to do the same as prometheus/prometheus, and you just forgot to merge forward.

If it was a deliberate choice to do only squashing, then we shouldn't just revert that choice on the fly. I leave it to you to reconsider (or not).

Besides all the other problems of squashing and rebasing (we are using Git instead of Subversion for a reason, right? ;), I find it super weird if the code I work with in main does not contain the commit that the whole world works with when they use the released version.

But as said, I don't want to open up that discussion just now.

@kakkoyun
Copy link
Member

kakkoyun commented Jul 6, 2022

To be honest, I hadn't foreseen any downside of squashing the commits until now. This showed me that it is a failed experiment. We continue squashing while merging for the PRs but it shouldn't be the only way. At least as a maintainer of this repo I would like to have that choice. We should remove that restriction.

What do you think? @bwplotka

@beorn7
Copy link
Member Author

beorn7 commented Jul 6, 2022

I think this deserves a wider discussion within the community (I think we should try to be consistent in this regard between repos, but even if the community as a whole doesn't agree, at least the decision that we don't need consistency should be a collective one ;). Thus, this is a perfect topic for the dev summit. I put a suggestion into the doc.

@bwplotka
Copy link
Member

bwplotka commented Jul 7, 2022

Sorry that this comes with a surprise and that we break your (and other users) existing workflows.

We just had some debate in DM with @beorn7, which touched on existential reasons and misunderstanding of Git, conventional semantics of commit and so on. While I still think we should squash everything for simplicity and buildable commits, squash-only logic needs redesign in our release process, which we did NOT do. For this reason, I would propose reenabling that option back it has proven to have a significant negative effect on Go modules.

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

7 participants