changes after adx-collector pprof (in prod) for CPU optimization - #1205
changes after adx-collector pprof (in prod) for CPU optimization#1205Elaa_jamazi (Elaa-jamazi) wants to merge 2 commits into
Conversation
d6cb3b7 to
7df7a06
Compare
|
|
||
| // Note: this cause allocations, but holding onto them in a pool causes a lot of memory to be used over time. | ||
| req := prompb.WriteRequestPool.Get() | ||
| defer func() { |
There was a problem hiding this comment.
This change unfortunately ignores the comment above that was removed. We should probably create the write requests directly instead of the pool so it's not as confusing, but this can be problematic. WriteRequests contain collections of objects which can contain many many objects, especially after restarts when senders flush a lot of data.
Retaining WriteRequests that have accumulated a lot of data can cause a collector to easily exceed its heap even when we don't need all of that space for subsequent operations later. We'll still be holding onto them within the pool.
There was a problem hiding this comment.
I addressed this issue. Now requests write request like you recommended: req := &prompb.WriteRequest{} + defer req.Reset() — allocates the outer struct directly (cheap; just slice headers) and still releases its inner TimeSeries / Label / Sample objects back to the bounded TimeSeriesPool via Reset()
There was a problem hiding this comment.
what if we add a boundedpool to avoid a pool causing a lot of memory to be used over time and to avoid the collection of objects concern with the WriteRequests? Each pooled TimeSeries is now capped at ~516 KiB (64 labels × 8 KiB max each, plus ~4 KiB of samples, if a Timeseries is oversized it's left for normal Go GC instead ). A new BoundedPool caps the pool to at most 1024 retained items via a fixed-capacity channel — anything beyond that is dropped instead of pooled. Multiplying these two hard limits (1024 × 516 KiB ≈ 530 MiB) gives a true worst-case ceiling, not an average, so the pool's memory footprint can never exceed that regardless of traffic burst size.
| enc := nativeLogsCSVWriterPool.Get(8 * 1024).(*transform2.NativeLogsCSVWriter) | ||
| defer nativeLogsCSVWriterPool.Put(enc) | ||
| defer func() { | ||
| enc.Reset() |
There was a problem hiding this comment.
This appears to add several different encoder resets with different lifecycles than we typically do. It also changes quite a lot of the logic in ways that I'm not very sure is safe. This store implementation is pretty heavily tested and benchmarked and deserves a lot of scrutiny.
There was a problem hiding this comment.
As we discussed on teams, I'll be removing the changes made to the log path, for now
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7df7a06 to
e16cbed
Compare
|
Elaa_jamazi (@Elaa-jamazi) please sign the CLA so we can review this change. Thanks. |
|
Elaa_jamazi (@Elaa-jamazi) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Summary
Optimizes CPU/allocation overhead in Collector, driven by pprof profiling of collector pods in production, which identified excessive per-request allocations in protobuf decode and remote-write handling as a top CPU contributor.
Changes
• pkg/prompb/protobuf.go , pkg/prompb/protobuf_test.go : Reuse Label / Sample / TimeSeries objects during protobuf decode instead of allocating fresh ones per request; return WriteRequest s and dropped series to pools after use.
• collector/metrics/handler.go , collector/metrics/handler_test.go : Wire pooled objects back through the write path so pooled requests are safely reused after a request completes.
• collector/scraper.go , collector/otlp/metrics.go : Adjust the scraper and OTLP ingestion paths to participate correctly in the pooling lifecycle (no double-pooling/use-after-reset).
• transform/transformer.go , transform/transformer_test.go : Preserve reusable label objects when transformations drop labels or series, avoiding unnecessary reallocation.