Hi, I was interested to see your comments; I have been looking over the last few months at the implementation of labels.Labels and labels.Builder. I have done quite a lot of work on Go performance optimisation, maybe I can help.
I took a look at https://github.com/prometheus/prometheus/pull/10662, but since it adds a hundred lines of code with no explanation, may I suggest that you add such explanation in comments or commit messages and then I can look again. Bryan Boreham On Thursday, 12 May 2022 at 04:56:43 UTC-7 [email protected] wrote: > Hi Brian, > > Thanks for your input ! > > >> A large part of the performance gains of Prometheus 2.x is attributed to >> it not using maps, so I would be extremely cautious about changing it for >> the sake of SD which is not the hot path. n is generally small for >> ingestion, so complexity alone can be misleading. >> > > Indeed, I was wrong. I tried to use a map in labels.Builder and we moved > from 7 CPUs to 12 CPUs used instantly. So I revert this change in the PR > immediately. > > But still in the particular case of the SD, using a map in the method > buildTargetLabels > <https://github.com/prometheus/prometheus/pull/10662/files#diff-f9baaa77ffadc6027a5881865a568d273ceac36a04528b6e6cb7a745bf95eac3R508> > helps > to gain around 1 to 2 CPUs on 9. (I tried first with labels.Builder). > > At the end, having a logic to not recompute the totality of the targets > but just a piece of that doesn't seem to work enough to see a huge drop of > the CPU usage. The PR I opened can still be merged even if the diff is not > that important. A combination of this PR and 10634 > <https://github.com/prometheus/prometheus/pull/10634> can help to > mitigate the issue. > But I'm afraid the only real fix is to have another channel in the SD that > will be used to receive a unique update of a target. In the particular case > of the k8s SD it will be used to FW the update of the PODs (for example). > > Augustin. > > Le jeu. 12 mai 2022 à 10:05, Brian Brazil <[email protected]> > a écrit : > >> On Wed, 11 May 2022 at 10:06, Augustin Husson <[email protected]> >> wrote: >> >>> Dear Prometheus developers, >>> >>> I'm currently working on trying to optimize the synchronization of the >>> targets in the Prometheus Discovery. (Issue is here >>> <https://github.com/prometheus/prometheus/issues/8014>) >>> >>> And after a couple of investigations and tests on a real environment, we >>> figured out that actually the usage of the method Set of the >>> labels.Builder >>> <https://github.com/prometheus/prometheus/blob/main/model/labels/labels.go#L388> >>> >>> cost a lot, and more generally the global usage of labels.Builder cost. >>> >>> In this PR <https://github.com/prometheus/prometheus/pull/10662> that >>> is supposed to fix the issue mentioned above, I started to replace the >>> usage of labels.Builder by a map and it immediately optimized the code. The >>> reason is because it reduces the complexity of the different methods. Like >>> for the method Set, we moved from o(n2) to o(n). >>> >>> So my question is: Would it be ok to change the internal implementation >>> of labels.Builder by using a map instead of multiple different slices ? It >>> won't change the API exposed, just the internal implementation. >>> >> >> A large part of the performance gains of Prometheus 2.x is attributed to >> it not using maps, so I would be extremely cautious about changing it for >> the sake of SD which is not the hot path. n is generally small for >> ingestion, so complexity alone can be misleading. >> >> When I last looked at the SD code the main performance issue looked to be >> that it had not been converted to using labels.Builder yet, and the >> continuous conversions from a map was what the main problem looked to be. >> Along with other low hanging fruit. >> >> I believe it will help to optimize smoothly (for example) the method >>> relabel.Process >>> <https://github.com/prometheus/prometheus/blob/main/model/relabel/relabel.go#L193> >>> >> >> This isn't on a hot path these days, as it's only run the first time we >> see a given sample. >> >> Brian >> >> >>> >>> Cheers, >>> Augustin. >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Prometheus Developers" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to [email protected]. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/prometheus-developers/CAOJizGe0AxLGw5G9BM1Z6Menamq%3DxTMHzFXjj73fTqM5%2BZgz5A%40mail.gmail.com >>> >>> <https://groups.google.com/d/msgid/prometheus-developers/CAOJizGe0AxLGw5G9BM1Z6Menamq%3DxTMHzFXjj73fTqM5%2BZgz5A%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> >> >> >> -- >> Brian Brazil >> www.robustperception.io >> > -- You received this message because you are subscribed to the Google Groups "Prometheus Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/prometheus-developers/0d12d44b-a429-46f8-a709-dc94a256b500n%40googlegroups.com.
