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
Drop old inmemory samples #13002
Merged
Merged
Drop old inmemory samples #13002
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
443511b
Drop old inmemory samples
marctc 5780421
Avoid copying timeseries when the feature is disabled
tpaschalis d5460dd
Run gofmt
tpaschalis 39edc4b
Clarify docs
marctc 255a6c0
Add more logging info
marctc 3da411a
Remove loggers
marctc 818ac24
optimize function and add tests
marctc f0c0129
Simplify filter
marctc 19f3275
rename var
marctc 1f7d6f9
Update help info from metrics
marctc 97cc3ce
use metrics to keep track of drop elements during buildWriteRequest
marctc 3620ce5
rename var in tests
marctc 1ffbe1a
pass time.Now as parameter
marctc ee89afe
Change buildwriterequest during retries
marctc 2836549
Revert "Remove loggers"
marctc d3775fc
use log level debug for loggers
marctc a99fc24
Merge branch 'main' into drop-old-inmemory-samples
tpaschalis 047a101
Fix linter
tpaschalis c0f1ef9
Remove noisy debug-level logs; add 'reason' label to drop metrics
tpaschalis 0b1c94e
Remove accidentally committed files
tpaschalis 0ecfa07
Propagate logger to buildWriteRequest to log dropped data
tpaschalis 488e1ff
Fix docs comment
tpaschalis 25ec6b6
Make drop reason more specific
tpaschalis 7dee9d0
Remove unnecessary pass of logger
tpaschalis 722825d
Use snake_case for reason label
tpaschalis 0ae385c
Merge branch 'main' of ssh://github.com/prometheus/prometheus into dr…
tpaschalis ec3e740
Fix dropped samples metric
tpaschalis File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @cstyan the use of a
*prometheus.CounterVec
here makes for a lot of difference in allocations.To showcase, here's the difference of running BenchmarkSampleSend on the current 7dee9d0 PR HEAD, and versus the same HEAD but with a simple
prometheus.Counter
instead.There is already a related fix out on client_golang for this, but will be released on the next v1.18.0 version and it's not currently being used by Prometheus.
When I updated client_golang to include the fix, it dramatically decreased allocations, making it on par with using the simple counter and is going a lot faster when it's discarding all samples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, I'm happy to see that there's
a) No performance regression for people not using the flag
b) No performance regression for people using the flag in normal operation, i.e. when all samples go through
c) A significant throughput gain when discarding unwanted samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is only impacting cases where we're throwing away samples (either via relabeling, due to their age or unintentionally), do you see this as a blocker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cstyan a new
v1.18.0
release of client_golang is out, which fixes the performance overhead. Do you think it's ok to bump as part of this release, or should we make it a different PR altogether?https://github.com/prometheus/client_golang/releases/tag/v1.18.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpaschalis prometheus should be updating that package soon, there's an automated PR open already for it. Lets just double check benchmark results in your PR after the package update is merged and then we can merge here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cstyan I could only find this automated PR, but it only bumped it in another subpackage and not the main go.mod file.
I've opened the following to manually bump it for the main go.mod and will re-run the benchmarks. #13373
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client_golang is now updated on main, and I've merged main into this branch to run the benchmarks once more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after updating client_golang, there's no impact on people not using the feature
as well as a speedup when samples are thrown away