Skip to content

Refactor part of queue_manger.go by creating struct to reuse some common function#17441

Merged
bwplotka merged 3 commits intoprometheus:mainfrom
pipiland2612:refactor_queue_manger
Nov 13, 2025
Merged

Refactor part of queue_manger.go by creating struct to reuse some common function#17441
bwplotka merged 3 commits intoprometheus:mainfrom
pipiland2612:refactor_queue_manger

Conversation

@pipiland2612
Copy link
Contributor

@pipiland2612 pipiland2612 commented Oct 31, 2025

@pipiland2612
create common struct and function to DRY
e1cb29b 
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
@pipiland2612
create timeSeriesStats to reduce return variable
9e6a626 
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
@pipiland2612
add timeSeriesAgeChecker to refactor filter code
704afd8 
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
@pipiland2612 pipiland2612 changed the title Refactor part of queue_manger.go by creating struc to reuse some common functon Refactor part of queue_manger.go by creating struct to reuse some common function Oct 31, 2025
@pipiland2612
Copy link
Contributor Author

pipiland2612 commented Nov 1, 2025 •
edited
Loading

Running some benchmark using the commands: go test -bench=BenchmarkBuildTimeSeries github.com/prometheus/prometheus/storage/remote -benchmem -count=5

On main branch:

goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/storage/remote
cpu: Apple M1 Pro
BenchmarkBuildTimeSeries-8   	     710	   1691550 ns/op	 2649832 B/op	   39747 allocs/op
BenchmarkBuildTimeSeries-8   	     711	   1669303 ns/op	 2649817 B/op	   39747 allocs/op
BenchmarkBuildTimeSeries-8   	     720	   1672789 ns/op	 2649830 B/op	   39747 allocs/op
BenchmarkBuildTimeSeries-8   	     718	   1671253 ns/op	 2649818 B/op	   39747 allocs/op
BenchmarkBuildTimeSeries-8   	     722	   1680717 ns/op	 2649825 B/op	   39747 allocs/op
PASS
ok  	github.com/prometheus/prometheus/storage/remote	93.375s

On this branch:


goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/storage/remote
cpu: Apple M1 Pro
BenchmarkBuildTimeSeries-8   	     718	   1683707 ns/op	 2649874 B/op	   39748 allocs/op
BenchmarkBuildTimeSeries-8   	     717	   1663382 ns/op	 2649868 B/op	   39748 allocs/op
BenchmarkBuildTimeSeries-8   	     710	   1753210 ns/op	 2649869 B/op	   39748 allocs/op
BenchmarkBuildTimeSeries-8   	     730	   1660716 ns/op	 2649887 B/op	   39748 allocs/op
BenchmarkBuildTimeSeries-8   	     724	   1665920 ns/op	 2649883 B/op	   39748 allocs/op
PASS
ok  	github.com/prometheus/prometheus/storage/remote	90.669s

Running benchstat to compare:


goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/storage/remote
cpu: Apple M1 Pro
                  │   old.txt    │            new.txt            │
                  │    sec/op    │    sec/op     vs base         │
BuildTimeSeries-8   1.673m ± ∞ ¹   1.666m ± ∞ ¹  ~ (p=0.548 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

                  │    old.txt    │               new.txt               │
                  │     B/op      │     B/op       vs base              │
BuildTimeSeries-8   2.527Mi ± ∞ ¹   2.527Mi ± ∞ ¹  +0.00% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

                  │   old.txt    │              new.txt               │
                  │  allocs/op   │  allocs/op    vs base              │
BuildTimeSeries-8   39.75k ± ∞ ¹   39.75k ± ∞ ¹  +0.00% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

So I think the performance is the same, which is good

bwplotka
bwplotka reviewed Nov 3, 2025
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Looks generally ok! Will review deeper later on

I love this part:

Image

bwplotka
bwplotka approved these changes Nov 13, 2025
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Great, thanks!

BTW you benchmarks like don't test the path of dropping samples, but that's fine. I don't see anything too bad in the code.

@bwplotka bwplotka merged commit 675bafe into prometheus:main Nov 13, 2025
29 checks passed
@pipiland2612 pipiland2612 deleted the refactor_queue_manger branch November 13, 2025 14:38
@perebaj perebaj mentioned this pull request Nov 13, 2025
Closed
@bwplotka bwplotka mentioned this pull request Dec 8, 2025
Closed
thampiotr
thampiotr reviewed Mar 2, 2026
View reviewed changes
storage/remote/queue_manager.go
s.qm.metrics.exemplarsTotal.Add(float64(exemplarCount))
s.qm.metrics.histogramsTotal.Add(float64(histogramCount))
s.qm.metrics.metadataTotal.Add(float64(metadataCount))
metricsUpdater.recordBatchAttempt(sc, begin)
Copy link
Contributor

@thampiotr thampiotr Mar 2, 2026

Choose a reason for hiding this comment

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

Because this is called before Store, won't the sentBatchDuration that this updates be incorrect?

@kgeckhart kgeckhart mentioned this pull request Mar 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwplotka bwplotka bwplotka approved these changes

@cstyan cstyan Awaiting requested review from cstyan cstyan is a code owner

@tomwilkie tomwilkie Awaiting requested review from tomwilkie tomwilkie is a code owner

+1 more reviewer

@thampiotr thampiotr thampiotr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pipiland2612 @bwplotka @thampiotr