Conversation
1b28a46 to
49ea7b0
Compare
This was referenced Jan 12, 2026
49ea7b0 to
1a80486
Compare
22 tasks
aknuds1
reviewed
Jan 13, 2026
Contributor
aknuds1
left a comment
There was a problem hiding this comment.
Just suggesting adding a doc comment for completeness.
…impl Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
1a80486 to
8a2921e
Compare
krajorama
reviewed
Jan 14, 2026
36fa046 to
8a2921e
Compare
Signed-off-by: bwplotka <bwplotka@gmail.com>
2fa1f39 to
051cdd6
Compare
bwplotka
commented
Jan 16, 2026
| i := len(a.a.pendingSamples) - 1 | ||
| for ; i >= 0; i-- { // Attach exemplars to the last matching sample. | ||
| if ref == storage.SeriesRef(a.a.pendingSamples[i].L.Hash()) { | ||
| if labels.Equal(l, a.a.pendingSamples[i].L) { |
Member
Author
There was a problem hiding this comment.
This is because some tests uses storage and storage does not use label hash, but atomic increment.
bwplotka
commented
Jan 16, 2026
| i := len(a.a.pendingSamples) - 1 | ||
| for ; i >= 0; i-- { // Attach metadata to the last matching sample. | ||
| if ref == storage.SeriesRef(a.a.pendingSamples[i].L.Hash()) { | ||
| if labels.Equal(l, a.a.pendingSamples[i].L) { |
Member
Author
There was a problem hiding this comment.
This is because some tests uses storage and storage does not use label hash, but atomic increment.
bwplotka
commented
Jan 16, 2026
| // As per AppenderV2 interface, opts.Exemplar slice is unsafe for reuse. | ||
| es = make([]exemplar.Exemplar, len(opts.Exemplars)) | ||
| copy(es, opts.Exemplars) | ||
| if a.a.appendExemplarsError != nil { |
Member
Author
There was a problem hiding this comment.
This is because in the previous code we were appending exemplar BEFORE we inject the error.
krajorama
reviewed
Jan 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds AppenderV2 support to additional storage implementations as part of the ongoing migration from Appender to AppenderV2 interface. It also includes fixes to the test storage mock for exemplar handling.
Changes:
- Added AppenderV2 implementations to remote WriteStorage, Storage, fanout, and readyStorage
- Fixed exemplar and metadata matching in test appender to use label comparison instead of ref-based matching
- Fixed exemplar error injection timing in test appender to occur before sample recording
- Added comprehensive tests for fanout AppenderV2 functionality
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| util/teststorage/appender.go | Fixed exemplar/metadata matching to use labels.Equal instead of ref comparison; moved exemplar error injection before recording |
| tsdb/head_append_v2.go | Added TODO comment for improving exemplar error messages |
| storage/remote/write.go | Added AppenderV2 method to WriteStorage with timestampTrackerV2 implementation; refactored common tracker fields into baseTimestampTracker |
| storage/remote/storage.go | Added AppenderV2 method that delegates to WriteStorage; added compile-time interface check |
| storage/interface_append.go | Added ErrOrNil and Handle helper methods to AppendPartialError; added documentation about duplicate exemplar handling |
| storage/interface.go | Updated Storage interface to require both Appendable and AppendableV2; added consistent deprecation warnings |
| storage/fanout_test.go | Added comprehensive tests for fanout AppenderV2 including error scenarios; added TestFanoutAppenderV2 and TestFanoutSelectSorted_AppenderV2 |
| storage/fanout.go | Added AppenderV2 method to fanout; implemented fanoutAppenderV2 with partial error aggregation; fixed Rollback to return error instead of nil |
| cmd/prometheus/main.go | Added AppenderV2 method to readyStorage with notReadyAppenderV2 implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: bwplotka <bwplotka@gmail.com>
051cdd6 to
f61a83b
Compare
This was referenced Jan 16, 2026
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.
Splitting work from #17675 into smaller PRs. This one starts requiring AppendableV2 implementation on ALL
storage.Storageimplementations; added AppendableV2 to the rest of easier implementations here. Where relevant AppendableV2 tests we added.I also adjusted appendable mock to:
Related to #17632
Chained on top of PART4a #17834 [merged]Does this PR introduce a user-facing change?