Skip to content

refactor(scrape)[PART2]: simplified scrapeLoop constructors & tests; add teststorage.Appendable mock#17631

Merged
bwplotka merged 14 commits intomainfrom
bwplotka/scrapeloop-ref
Dec 22, 2025
Merged

refactor(scrape)[PART2]: simplified scrapeLoop constructors & tests; add teststorage.Appendable mock#17631
bwplotka merged 14 commits intomainfrom
bwplotka/scrapeloop-ref

Conversation

@bwplotka
Copy link
Member

@bwplotka bwplotka commented Dec 1, 2025 •
edited
Loading

Related to #17632

This PR:

  • scrape: Refactors scrapeLoop constructors to reduce the horror of ~30 parameter function.
See the scrapeLoop construction diff

Before

image

After

image

Benchmarks

benchstat append-v1.txt append.scrapeloop-ref.txt 
goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/scrape
cpu: Apple M4 Pro
                                                      │ append-v1.txt │     append.scrapeloop-ref.txt      │
                                                      │    sec/op     │    sec/op     vs base              │
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromText-2      331.9µ ± 17%   330.3µ ± 16%       ~ (p=0.394 n=6)
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=OMText-2        330.1µ ± 10%   335.4µ ±  9%       ~ (p=0.310 n=6)
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromProto-2     415.1µ ± 11%   405.9µ ±  1%  -2.22% (p=0.041 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromText-2     512.5µ ±  3%   516.1µ ±  2%       ~ (p=0.310 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=OMText-2       508.6µ ±  6%   506.4µ ±  5%       ~ (p=0.699 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromProto-2    497.4µ ±  5%   504.7µ ±  3%       ~ (p=0.310 n=6)
geomean                                                  425.0µ         425.5µ        +0.10%

                                                      │ append-v1.txt │      append.scrapeloop-ref.txt      │
                                                      │     B/op      │     B/op       vs base              │
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromText-2      347.1Ki ± 2%   343.0Ki ±  4%       ~ (p=0.132 n=6)
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=OMText-2        346.1Ki ± 6%   351.3Ki ±  3%       ~ (p=0.589 n=6)
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromProto-2     454.6Ki ± 5%   449.9Ki ±  1%       ~ (p=0.699 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromText-2     323.4Ki ± 0%   323.4Ki ± 10%       ~ (p=0.545 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=OMText-2       323.5Ki ± 4%   323.5Ki ±  3%       ~ (p=0.939 n=6)
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromProto-2    398.1Ki ± 2%   397.3Ki ±  3%       ~ (p=0.805 n=6)
geomean                                                  362.7Ki        362.1Ki        -0.16%

                                                      │ append-v1.txt │      append.scrapeloop-ref.txt      │
                                                      │   allocs/op   │  allocs/op   vs base                │
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromText-2        12.00 ± 0%    12.00 ± 0%       ~ (p=1.000 n=6) ¹
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=OMText-2          13.00 ± 0%    13.00 ± 0%       ~ (p=1.000 n=6) ¹
ScrapeLoopAppend/data=1Fam1000Gauges/fmt=PromProto-2       28.00 ± 0%    28.00 ± 0%       ~ (p=1.000 n=6) ¹
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromText-2       13.00 ± 0%    13.00 ± 0%       ~ (p=1.000 n=6) ¹
ScrapeLoopAppend/data=237FamsAllTypes/fmt=OMText-2         14.00 ± 0%    14.00 ± 0%       ~ (p=1.000 n=6) ¹
ScrapeLoopAppend/data=237FamsAllTypes/fmt=PromProto-2     2.231k ± 0%   2.231k ± 0%       ~ (p=1.000 n=6) ¹
geomean                                                    34.79         34.79       +0.00%
¹ all samples are equal

benchstat scrapeAndReport-v1.txt scrapeAndReport-scrapeloop-ref.txt 
goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/scrape
cpu: Apple M4 Pro
                            │ scrapeAndReport-v1.txt │ scrapeAndReport-scrapeloop-ref.txt │
                            │         sec/op         │      sec/op        vs base         │
ScrapeLoopScrapeAndReport-2              564.5µ ± 2%         555.6µ ± 1%  ~ (p=0.180 n=6)

                            │ scrapeAndReport-v1.txt │ scrapeAndReport-scrapeloop-ref.txt │
                            │          B/op          │       B/op         vs base         │
ScrapeLoopScrapeAndReport-2             7.233Ki ± 1%        7.271Ki ± 0%  ~ (p=0.132 n=6)

                            │ scrapeAndReport-v1.txt │ scrapeAndReport-scrapeloop-ref.txt │
                            │       allocs/op        │  allocs/op    vs base              │
ScrapeLoopScrapeAndReport-2               97.00 ± 0%     98.00 ± 0%  +1.03% (p=0.002 n=6)

benchstat restartLoops-v1.txt restartLoops.v3.txt        
goos: darwin
goarch: arm64
pkg: github.com/prometheus/prometheus/scrape
cpu: Apple M4 Pro
                         │ restartLoops-v1.txt │        restartLoops.v3.txt         │
                         │       sec/op        │   sec/op     vs base               │
ScrapePoolRestartLoops-2           1.806m ± 4%   1.620m ± 2%  -10.29% (p=0.002 n=6)

                         │ restartLoops-v1.txt │         restartLoops.v3.txt         │
                         │        B/op         │     B/op      vs base               │
ScrapePoolRestartLoops-2          2.389Mi ± 0%   1.923Mi ± 0%  -19.49% (p=0.002 n=6)

                         │ restartLoops-v1.txt │        restartLoops.v3.txt         │
                         │      allocs/op      │  allocs/op   vs base               │
ScrapePoolRestartLoops-2           38.01k ± 0%   31.00k ± 0%  -18.42% (p=0.002 n=6)

Does this PR introduce a user-facing change?

NONE

@bwplotka bwplotka mentioned this pull request Dec 1, 2025
Closed
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch from c436233 to fd6a28a Compare December 1, 2025 12:46
@bwplotka bwplotka marked this pull request as draft December 1, 2025 12:46
@bwplotka bwplotka mentioned this pull request Dec 1, 2025
Open
22 tasks
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch 4 times, most recently from 8fee513 to 5be9332 Compare December 11, 2025 10:48
bwplotka
bwplotka commented Dec 11, 2025
View reviewed changes
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch 11 times, most recently from 1a2a734 to 06bd846 Compare December 12, 2025 09:34
bwplotka
bwplotka commented Dec 12, 2025
View reviewed changes
bwplotka
bwplotka commented Dec 12, 2025
View reviewed changes
bwplotka
bwplotka commented Dec 12, 2025
View reviewed changes
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch from 06bd846 to 1b8e49e Compare December 12, 2025 09:43
@bwplotka bwplotka changed the title refactor(scrape): simplified scrapeLoop constructors & tests; add teststorage.Appender mock refactor(scrape)[PART2]: simplified scrapeLoop constructors & tests; add teststorage.Appendable mock Dec 12, 2025
bwplotka
bwplotka commented Dec 12, 2025
View reviewed changes
bwplotka
bwplotka commented Dec 12, 2025
View reviewed changes
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch from 39b5dcd to 38c991b Compare December 17, 2025 04:34
@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025 •
edited
Loading

Running some benchmarks, putting the latest results in PR description.

Microbenchmarks (added few more) are the same and even 20% better for restarting scrape loops (target changes)

Prombench:

  • Allocs were slightly worse in the beginning but after initial period it stabilized and this PR even allocs less:
image
  • RSS is same
image
  • CPU is 0.05 CPU/s worse in the beginning then stabilizes
image

CPU increase might be explained by higher HTTP load taken by PR Prometheus

image

It looks.. good to go?

@bwplotka
added extra benchmark for restartLoops
75651e8 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch from 38c991b to 75651e8 Compare December 17, 2025 04:47
@bwplotka
addressed last comments
c97952c 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka
fix TestConcurrentAppender_ReturnsErrAppender naming
c264515 
Signed-off-by: bwplotka <bwplotka@gmail.com>
krajorama
krajorama reviewed Dec 17, 2025
View reviewed changes
@bwplotka
addressed small comments
782cfb0 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka
refactor(scrape): ensure scrape config is reloaded; added test
5c80c86 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka force-pushed the bwplotka/scrapeloop-ref branch from 00f9fb0 to 5c80c86 Compare December 17, 2025 13:37
@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025

/prombench cancel

@prombot
Copy link
Contributor

prombot commented Dec 17, 2025

Benchmark cancel is in progress.

bwplotka
bwplotka commented Dec 17, 2025
View reviewed changes
@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025

/prombench main

@prombot
Copy link
Contributor

prombot commented Dec 17, 2025

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-17631 and main

After the successful deployment (check status here), the benchmarking results can be viewed at:

Available Commands:

  • To restart benchmark: /prombench restart main
  • To stop benchmark: /prombench cancel
  • To print help: /prombench help

@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025

Updated microbenchmarks in PR description, no change, similarly good results.

@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025

Prombench results are good, 1:1.

Be careful when using Grafana links from prombench comments e.g. #17631

Grafana keeps caching wrong dashboard variable, I was confused a few times already. Ensure var-pr-number=17631 var.

@bwplotka
Copy link
Member Author

bwplotka commented Dec 17, 2025

This is good for final (?) review @krajorama @bboreham

@bwplotka bwplotka requested a review from krajorama December 18, 2025 08:05
@bboreham
Copy link
Member

bboreham commented Dec 19, 2025

Grafana keeps caching wrong dashboard variable

The problem is that the generated url is wrong:

http://prombench.prometheus.io/grafana/d/7gmLoNDmz/prombench?orgId=1&var-pr-number=17631

This works:

http://prombench.prometheus.io/grafana/d/7gmLoNDmz/prombench?orgId=1&var-prNumber=17631

I tried to see if this changed recently but I can't follow the history.

krajorama
krajorama approved these changes Dec 22, 2025
View reviewed changes
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Approved with two nit comments. With such a big change, I would not be surprised if we missed something though.

@bwplotka
addressed comments.
c53451a 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka enabled auto-merge (squash) December 22, 2025 09:12
@bwplotka bwplotka merged commit 17e06db into main Dec 22, 2025
46 checks passed
@bwplotka bwplotka deleted the bwplotka/scrapeloop-ref branch December 22, 2025 09:38
@bwplotka
Copy link
Member Author

bwplotka commented Dec 22, 2025

/prombench cancel

@prombot
Copy link
Contributor

prombot commented Dec 22, 2025

Benchmark cancel is in progress.

@aknuds1 aknuds1 mentioned this pull request Dec 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aknuds1 aknuds1 aknuds1 approved these changes

@krajorama krajorama krajorama approved these changes

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

@ywwg ywwg Awaiting requested review from ywwg

@bboreham bboreham Awaiting requested review from bboreham

Assignees

No one assigned

Labels

prombench

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bwplotka @prombot @krajorama @bboreham @aknuds1