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

Store Prometheus labels as a slice instead of a map #3422

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Nov 5, 2024

Description:
Store labels for targets in the target allocator the same way Prometheus itself does - as a key+value list of type labels.Labels instead of a model.Labelset map. The former is what relabelling acts on, so we get to skip the conversion there, and building the list is faster as well. This makes looking up values of specific labels slower, but we don't really do that outside of determining the Node name.

I've modified the benchmark to actually drop half the targets during relabelling to make it more realistic.

Benchstat shows some nice gains:

goos: linux
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator
cpu: AMD Ryzen 9 7950X3D 16-Core Processor          
                                                      │ bench_targets_main.txt │      bench_targets_branch.txt       │
                                                      │         sec/op         │   sec/op     vs base                │
ProcessTargets/least-weighted-32                                   78.24m ± 1%   42.75m ± 1%  -45.36% (p=0.000 n=10)
ProcessTargets/consistent-hashing-32                               78.86m ± 0%   42.72m ± 1%  -45.83% (p=0.000 n=10)
ProcessTargets/per-node-32                                         78.71m ± 0%   42.86m ± 1%  -45.55% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/per-node-32                        78.69m ± 1%   43.15m ± 1%  -45.17% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/least-weighted-32                  79.11m ± 1%   43.25m ± 1%  -45.33% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/consistent-hashing-32              79.05m ± 1%   42.86m ± 2%  -45.79% (p=0.000 n=10)
geomean                                                            78.78m        42.93m       -45.50%

                                                      │ bench_targets_main.txt │       bench_targets_branch.txt       │
                                                      │          B/op          │     B/op      vs base                │
ProcessTargets/least-weighted-32                                  45.14Mi ± 0%   40.41Mi ± 0%  -10.48% (p=0.000 n=10)
ProcessTargets/consistent-hashing-32                              45.15Mi ± 0%   40.41Mi ± 0%  -10.49% (p=0.000 n=10)
ProcessTargets/per-node-32                                        45.15Mi ± 0%   40.41Mi ± 0%  -10.49% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/per-node-32                       45.38Mi ± 0%   40.44Mi ± 0%  -10.88% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/least-weighted-32                 45.38Mi ± 0%   40.44Mi ± 0%  -10.88% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/consistent-hashing-32             45.38Mi ± 0%   40.44Mi ± 0%  -10.88% (p=0.000 n=10)
geomean                                                           45.26Mi        40.43Mi       -10.68%

                                                      │ bench_targets_main.txt │      bench_targets_branch.txt       │
                                                      │       allocs/op        │  allocs/op   vs base                │
ProcessTargets/least-weighted-32                                  112.36k ± 0%   52.05k ± 0%  -53.67% (p=0.000 n=10)
ProcessTargets/consistent-hashing-32                              112.38k ± 0%   52.05k ± 0%  -53.68% (p=0.000 n=10)
ProcessTargets/per-node-32                                        112.38k ± 0%   52.06k ± 0%  -53.68% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/per-node-32                       112.73k ± 0%   52.26k ± 0%  -53.64% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/least-weighted-32                 112.73k ± 0%   52.26k ± 0%  -53.64% (p=0.000 n=10)
ProcessTargetsWithRelabelConfig/consistent-hashing-32             112.74k ± 0%   52.26k ± 0%  -53.64% (p=0.000 n=10)
geomean                                                            112.6k        52.16k       -53.66%

swiatekm and others added 2 commits November 5, 2024 20:21
This should be more efficient in general, and will let us skip target
hashing completely in the future.
@swiatekm swiatekm added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 5, 2024
@swiatekm swiatekm marked this pull request as ready for review November 5, 2024 20:32
@swiatekm swiatekm requested a review from a team as a code owner November 5, 2024 20:32
@nicolastakashi
Copy link
Contributor

Amazing work on that @swiatekm 👏🏽

Copy link
Contributor

@nicolastakashi nicolastakashi left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolastakashi
Copy link
Contributor

Hey @swiatekm anything I can do to help with this change?

@swiatekm
Copy link
Contributor Author

@nicolastakashi I'm just waiting for another approver review.

Copy link
Contributor

@yuriolisa yuriolisa left a comment

Choose a reason for hiding this comment

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

LGTM

@swiatekm swiatekm merged commit 1108621 into open-telemetry:main Nov 12, 2024
38 checks passed
@swiatekm swiatekm deleted the feat/ta-store-labels-slice branch November 12, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants