Skip to content

feat(scrape)[PART5b]: Add AppenderV2 support to scrape.NewManager constructor#17872

Merged
bwplotka merged 4 commits intomainfrom
bwplotka/a2-scrape-2
Jan 23, 2026
Merged

feat(scrape)[PART5b]: Add AppenderV2 support to scrape.NewManager constructor#17872
bwplotka merged 4 commits intomainfrom
bwplotka/a2-scrape-2

Conversation

@bwplotka
Copy link
Member

@bwplotka bwplotka commented Jan 16, 2026 •
edited
Loading

Progresses the #17632

This small PR allows NewManager users to use AppendableV2. Does not switch prod yet.

This PR allows OpenTelemetry Collector use new interface to see how much it improves (or not) the Otel collector complexity and efficiency with prometheusreceiver cc @dashpole @aknuds1

Does this PR introduce a user-facing change?

NONE

@bwplotka bwplotka requested a review from Copilot January 16, 2026 11:54
This was referenced Jan 16, 2026
Closed
Open
@bwplotka
Copy link
Member Author

bwplotka commented Jan 16, 2026

Windows TestQueryLog is flaky, see https://cloud-native.slack.com/archives/C01AUBA4PFE/p1768564859437119

@bwplotka bwplotka requested review from aknuds1 and krajorama January 16, 2026 12:03
Copilot AI reviewed Jan 16, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for AppendableV2 to scrape.NewManager as part of the ongoing migration from Appendable to AppendableV2. The changes allow users to optionally specify AppendableV2 when creating a scrape manager, while maintaining backward compatibility by still accepting the legacy Appendable interface.

Changes:

  • Modified scrape.NewManager signature to accept both appendable and appendableV2 parameters
  • Added validation to prevent both parameters from being specified simultaneously
  • Updated all test code and production code to use the new signature
  • Removed workaround code from AppendV2 tests that manually set private fields

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scrape/manager.go Updated NewManager to accept both Appendable and AppendableV2 parameters with validation
scrape/scrape_test.go Updated test to pass nil for appendableV2 parameter
scrape/scrape_append_v2_test.go Updated to use appendableV2 parameter and removed workaround code
scrape/manager_test.go Updated all test calls and runManagers helper to support both parameters
cmd/prometheus/main.go Updated production call with nil for appendableV2 (migration pending)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bwplotka bwplotka changed the title feat(scrape)[PART5b]: Add AppenderV2 support to scrape.NewManager optionally to V1 feat(scrape)[PART5b]: Add AppenderV2 support to scrape.NewManager constructor Jan 16, 2026
Copilot AI mentioned this pull request Jan 16, 2026
Closed
@prometheus prometheus deleted a comment from Copilot AI Jan 16, 2026
@bwplotka bwplotka mentioned this pull request Jan 16, 2026
Merged
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-1 branch 18 times, most recently from 21315c2 to 49c60b0 Compare January 20, 2026 12:54
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-1 branch from 49c60b0 to 28be7e7 Compare January 20, 2026 13:27
Base automatically changed from bwplotka/a2-scrape-1 to main January 21, 2026 08:21
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-2 branch from a939a0f to 9e1d128 Compare January 21, 2026 08:50
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-2 branch from 9e1d128 to 067f5f8 Compare January 21, 2026 08:51
bwplotka
bwplotka commented Jan 21, 2026
View reviewed changes
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-2 branch from 067f5f8 to e40df21 Compare January 21, 2026 09:09
@bwplotka bwplotka mentioned this pull request Jan 21, 2026
Merged
@bwplotka
Copy link
Member Author

bwplotka commented Jan 21, 2026 •
edited
Loading

Failed tests are due to notifier flakiness #17899 - rebased.

bwplotka and others added 3 commits January 22, 2026 11:29
@bwplotka
feat(scrape)[PART5b]: Add AppenderV2 support to scrape.NewManager opt…
76fb486 
…ionally to V1

Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka
Update scrape/manager.go
2fbe4be 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka
fixes after rebase
0c6a269 
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka force-pushed the bwplotka/a2-scrape-2 branch from e40df21 to 0c6a269 Compare January 22, 2026 11:29
@bwplotka bwplotka requested a review from ArthurSens January 22, 2026 11:30
aknuds1
aknuds1 approved these changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM modulo typos.

@ArthurSens ArthurSens mentioned this pull request Jan 22, 2026
Closed
@ArthurSens
Copy link
Member

ArthurSens commented Jan 22, 2026 •
edited
Loading

Hey 👋 --

I've tried out the commit SHA from this PR in the collector just to have a glance at how much the code could be simplified with the new interface. The PR can be seen here: open-telemetry/opentelemetry-collector-contrib#45597

Overall, the codebase looks a lot more readable and easier to navigate. There was a lot of state handling for the v1 appender, e.g. when handling histogram ingestion or when deciding which suffixes to remove based on metadata.Type (the collector has functionality to remove suffixes from metric names).

Another awesome improvement is that in v1 we were forced to have an intermediate struct between what the appender gives us and what we provide to the next consumer in the collector pipeline. With appenderv2, we can generate pmetric.Metrics directly from the Append method. If you compare the Commit implementation for both appenders, you'll immediately notice how much simpler the v2 implementation is ❤️

Things that aren't working so well yet are:

  • Metadata that is returned from Appendv2 seems to be empty every time.
  • Seems like state handling is still needed for summaries.

I'll be honest and say that I haven't been as invested in this refactoring as other folks here, so I'm not sure if I had different expectations for the metadata and summary problems, but I'd be happy to continue providing feedback from the collector side

@bwplotka
Copy link
Member Author

bwplotka commented Jan 23, 2026

Thanks for looking so quickly on this! 🤗 Looks like it fulfills the mission! Also benchmark results should look sweet.

I wished we could see the lines removed impact of it, but I understand you want to feature gate the switch (do you need to? no functionality changed). This is actually an important decision because supporting both V1 and V2 Appender implementations is extremely complex. Ideally we remove V1 ASAP. How long do you think we need to support this for Collector? 2mo-6mo is fine, but eventually we need to remove that huge tech debt.

We can remove some metadata caches at some point with #17619 to reduce allocs a lot on collector even more (and Prometheus side)

Metadata that is returned from Appendv2 seems to be empty every time.

Yes, you need to adjust your scrape opts. Explained in the comment: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/45597/changes#r2720171281

Seems like state handling is still needed for summaries.

Well, also classic histograms. Or do you strictly use NHCB? 🎉 If you are on NHCB, and state handling is an overhead, then sounds like we have a strong motivation to push forward NativeSummaries, at least on Appender level.

@bwplotka @aknuds1
Apply suggestions from code review
e626107 
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka bwplotka enabled auto-merge (squash) January 23, 2026 08:41
@bwplotka bwplotka merged commit bec7022 into main Jan 23, 2026
50 checks passed
@bwplotka bwplotka deleted the bwplotka/a2-scrape-2 branch January 23, 2026 09:04
@ArthurSens
Copy link
Member

ArthurSens commented Jan 23, 2026

Indeed, benchmarks are showing improvements (we're doing more work on append, but a lot less on commit):

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/prometheusreceiver/internal
cpu: Apple M2 Pro
                                               │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v1_appendcommit_1s.txt │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v2_appendcommit_1s.txt │
                                               │                                                    sec/op                                                    │                                        sec/op                                          vs base               │
AppendCommit/ClassicMetrics/Baseline-12                                                                                                          65.88m ± 11%                                                                            51.22m ± 12%  -22.24% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithTargetInfo-12                                                                                                    65.03m ±  1%                                                                            51.49m ± 12%  -20.82% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithScopeInfo-12                                                                                                     64.99m ±  7%                                                                            51.43m ±  2%  -20.86% (p=0.002 n=6)
AppendCommit/NativeHistogram/Baseline-12                                                                                                         68.11m ±  3%                                                                            52.53m ±  1%  -22.88% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithTargetInfo-12                                                                                                   68.09m ±  3%                                                                            52.47m ±  3%  -22.94% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithScopeInfo-12                                                                                                    68.54m ±  1%                                                                            52.39m ±  2%  -23.56% (p=0.002 n=6)
geomean                                                                                                                                          66.75m                                                                                  51.92m        -22.22%

                                               │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v1_appendcommit_1s.txt │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v2_appendcommit_1s.txt │
                                               │                                                     B/op                                                     │                                         B/op                                           vs base               │
AppendCommit/ClassicMetrics/Baseline-12                                                                                                          36.65Mi ± 0%                                                                            29.72Mi ± 0%  -18.90% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithTargetInfo-12                                                                                                    36.65Mi ± 0%                                                                            29.72Mi ± 0%  -18.90% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithScopeInfo-12                                                                                                     36.65Mi ± 0%                                                                            29.72Mi ± 0%  -18.90% (p=0.002 n=6)
AppendCommit/NativeHistogram/Baseline-12                                                                                                         39.39Mi ± 0%                                                                            32.47Mi ± 0%  -17.58% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithTargetInfo-12                                                                                                   39.40Mi ± 0%                                                                            32.47Mi ± 0%  -17.58% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithScopeInfo-12                                                                                                    39.40Mi ± 0%                                                                            32.47Mi ± 0%  -17.58% (p=0.002 n=6)
geomean                                                                                                                                          38.00Mi                                                                                 31.07Mi       -18.24%

                                               │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v1_appendcommit_1s.txt │ /Users/arthursens/Documents/projetos/opentelemetry-collector-contrib/benchmarks/bench_v2_appendcommit_1s.txt │
                                               │                                                  allocs/op                                                   │                                       allocs/op                                        vs base               │
AppendCommit/ClassicMetrics/Baseline-12                                                                                                           730.2k ± 0%                                                                             610.2k ± 0%  -16.43% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithTargetInfo-12                                                                                                     730.2k ± 0%                                                                             610.2k ± 0%  -16.43% (p=0.002 n=6)
AppendCommit/ClassicMetrics/WithScopeInfo-12                                                                                                      730.2k ± 0%                                                                             610.2k ± 0%  -16.43% (p=0.002 n=6)
AppendCommit/NativeHistogram/Baseline-12                                                                                                          760.2k ± 0%                                                                             640.2k ± 0%  -15.79% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithTargetInfo-12                                                                                                    760.2k ± 0%                                                                             640.2k ± 0%  -15.79% (p=0.002 n=6)
AppendCommit/NativeHistogram/WithScopeInfo-12                                                                                                     760.2k ± 0%                                                                             640.2k ± 0%  -15.79% (p=0.002 n=6)
geomean                                                                                                                                           745.0k                                                                                  625.0k       -16.11%

Bartek and I chatted briefly on Slack, and it seems the collector won't be able to eliminate state handling completely; we still need it to parse summaries correctly. For classic histograms, we're thinking about hardcoding the scrape setting that transforms classic histograms into NHCB, which means we could get rid of state handling for histograms at least :)

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

Reviewers

Copilot code review Copilot Copilot left review comments

@aknuds1 aknuds1 aknuds1 approved these changes

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

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

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

@ArthurSens ArthurSens Awaiting requested review from ArthurSens

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bwplotka @ArthurSens @aknuds1