refactor(appenderV2)[PART5a]: add AppendableV2 support to scrape loop + tests#17867
Merged
refactor(appenderV2)[PART5a]: add AppendableV2 support to scrape loop + tests#17867
Conversation
766d76d to
ccbe71a
Compare
bwplotka
commented
Jan 15, 2026
7e37958 to
b24934e
Compare
22 tasks
b24934e to
4eb649a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds AppendableV2 support to the scrape package to progress towards a new append API. The change is backward-compatible and does not modify production behavior yet, as it introduces parallel V2 code paths alongside existing V1 paths.
Changes:
- Adds AppendableV2 support to scrapePool and scrapeLoop with runtime selection based on which appendable is configured
- Implements V2 versions of limiting appenders (limitAppenderV2, timeLimitAppenderV2, bucketLimitAppenderV2, maxSchemaAppenderV2)
- Duplicates comprehensive test suite with "_AppendV2" suffix to validate V2 flow (tests will be removed in M2 stage)
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scrape/scrape.go | Adds appendableV2 fields to scrapePool and scrapeLoop; updates appender() to select V2 or V1 path at runtime |
| scrape/scrape_append_v2.go | Implements scrapeLoopAppenderV2 and V2 limiting appenders with new Append signature |
| scrape/target.go | Adds V2 versions of limiting appenders (limitAppenderV2, timeLimitAppenderV2, bucketLimitAppenderV2, maxSchemaAppenderV2) |
| scrape/manager.go | Updates Manager to support appendableV2 and passes it to newScrapePool |
| scrape/scrape_test.go | Updates existing tests to pass nil for new appendableV2 parameter in newScrapePool calls |
| scrape/scrape_append_v2_test.go | Comprehensive test duplication for V2 flow (4630 lines, to be removed in M2) |
| scrape/helpers_test.go | Adds withAppendableV2 test helper and removes default appendable from newTestScrapeLoop |
| util/teststorage/appender.go | Fixes ref computation logic to handle chained appendables with different ref logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
051cdd6 to
f61a83b
Compare
4eb649a to
96bdd68
Compare
This was referenced Jan 16, 2026
Member
Author
|
Scraping is ~6% slower for V2 flow. I will investigate where we lose those CPU cycles, I think we shouldn't see that much overhead (although 5% is maybe small) - probably next week. |
krajorama
reviewed
Jan 16, 2026
Member
krajorama
left a comment
There was a problem hiding this comment.
The logic seems to be good, but I'm really surprised by the testing strategy, especially for the scrape.
Most of the tests go through the scrapeLoopAppendAdapter interface which is common for v1 and v2, so you don't have to actually duplicate tests, just make the test function take the scrapeLoopAppendAdapter as an argument and run it twice. Also scrape_append_v2_test.go has a bunch of tests that don't use v2 at all, just in the name?
I think we should not have scrape_append_v2_test.go , but add the cases to scrape_test.go. The current way of working is very time consuming to actually check.
Member
Author
It's consistent with TSDB and Agent DB. I see how test cases would be easier, especially after refactor we did. I did follow test case logic in #17872 - easier to review for sure. Let me invest time into it. |
4198598 to
126cfab
Compare
369ee03 to
429793e
Compare
429793e to
88a6b80
Compare
bwplotka
commented
Jan 19, 2026
|
|
||
| // In majority cases we can trust that the current series/histogram is matching the lastMeta and lastMFName. | ||
| // However, optional TYPE etc metadata and broken OM text can break this, detect those cases here. | ||
| if !isSeriesPartOfFamily(lset.Get(model.MetricNameLabel), lastMFName, lastMeta.Type) { |
Member
Author
There was a problem hiding this comment.
This is one source of the overhead we saw in microbenchmarks (CPU). Notably, to pass correct metadata to V2 we need to check for that broken text format (at least to maintain parity). V1 was only doing this when metadata changes (the broken on - as-is - not really connected to the sample we append)
Microbenchmarks bench appendMetadataToWAL.
This is only for a broken feature, so I'd change / add case microbenchmarks to skip appendMetadataToWAL feature for now, but we probably want to do a move to per parser and rethink this path in general. I am also curious how type and unit handle this (likely they don't) for always-on metadata 🤔
Member
Author
There was a problem hiding this comment.
I added cases for benchmark.
Keeping this path slower for now, I think it's acceptable given metadata-wal-records has bigger issues. Perhaps worth to add issue and TODO to optimize this (move to parsers)
krajorama
reviewed
Jan 19, 2026
Member
krajorama
left a comment
There was a problem hiding this comment.
some nit comments, this is looking much better, thank you
0615990 to
1c0c729
Compare
1d57f4d to
21315c2
Compare
Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
21315c2 to
49c60b0
Compare
Member
Author
|
The latest benchmark for completeness: The I am still trying understand why mem and CPU regress (appendMetadataToWAL=false). |
1 task
Signed-off-by: bwplotka <bwplotka@gmail.com>
49c60b0 to
28be7e7
Compare
krajorama
approved these changes
Jan 20, 2026
| // TODO(bwplotka): We should likely reorder only within a group of sequential NaNs or only in scrape package. | ||
| func RequireEqual(t testing.TB, expected, got []Sample, msgAndArgs ...any) { | ||
| opts := []cmp.Option{sampleComparer} | ||
| if includeStaleNaNs(expected) { |
Member
There was a problem hiding this comment.
I don't like this at all. It feels like a bug waiting to happen, by obscuring the real ordering. Still it's better than the original that did this unconditionally. I have a better idea how to do this, will open PR.
Member
Author
|
Checked all failures - those are related to notifier flakiness: 
Thus, merging. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Progresses the #17632
Chained on top of #17835 (comment)Majority of changed LOCs is the duplicated tests for V2 flow. Will be removed in M2 stage.
This PR:
Should not change behavior on e2e level. 2 minor changes to test expectations were added to literally 2 tests to accommodate Appender new contract:
func TestScrapeAppend_MetadataUpdatetests.TestScrapeLoopAppendExemplarextra processing of expected samples.Does this PR introduce a user-facing change?