[RW2] Fix: Only update metadata to WAL when metadata-wal-records feature is enabled#17470
Conversation
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
bwplotka
left a comment
There was a problem hiding this comment.
LGTM.
NOTE: This is to match
Line 1895 in ad7d1ae
I wonder if OTLP receiving does not have the same bug:
bwplotka
left a comment
There was a problem hiding this comment.
Actually we need a different flag (:
|
Please also update title and description - it's not type and unit flag |
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
bwplotka
left a comment
There was a problem hiding this comment.
Thanks!
Hi @bwplotka, thanks for reminding me. I was quite also questioning whether it is type-and-unit-labels or metadata-wal-records when I was first doing this PR
So what made you to eventually decide on type and unit? What made you to ignore your intuition here? (:
Err, because I see the type-and-unit feature already there so may be it is what we're wanting :-) |
## Summary This PR upgrades the `github.com/prometheus/prometheus` dependency from v0.307.3 to v0.308.0. ## Changes Made #### Fixed `RemoteWriteProtoMsg` unrecognized In prometheus/prometheus v0.308.0, the remote write protocol message types were moved from `github.com/prometheus/prometheus/config` to `github.com/prometheus/client_golang/exp/api/remote` as part of [prometheus/prometheus#17197](prometheus/prometheus#17197). #### Fixed `CreatedTimestamp` → `StartTimestamp` In [prometheus/prometheus#17411](prometheus/prometheus#17411), the Remote Write 2.0 spec was updated to 2.0-rc.4. The `CreatedTimestamp` field was renamed to `StartTimestamp` and moved from `TimeSeries` to individual `Sample` and `Histogram` messages. #### Fixed not enough arguments in`api_v1.NewAPI` The `api_v1.NewAPI` function signature changed due to [prometheus/prometheus#17470](prometheus/prometheus#17470) - Added `appendMetadata bool` parameter for remote write metadata handling. Added the new parameter `appendMetadata = false` - prometheusreceiver scrapes targets, doesn't receive remote write requests ## Remaining Issues ### `EnableNativeHistogramsIngestion` removed from `scrape.Options` There's one change in this upgrade that I'd love to get the community's input on before finalizing. In [prometheus/prometheus#17315](prometheus/prometheus#17315), Prometheus removed the `EnableNativeHistogramsIngestion` field from `scrape.Options`. Native histogram ingestion is now controlled through the Prometheus scrape config option `scrape_native_histograms` rather than programmatically. This creates an interesting situation for us. Currently, we have the feature gate `receiver.prometheusreceiver.EnableNativeHistograms` that controls whether native histograms are converted to OTel exponential histograms. But with this Prometheus change, the *ingestion* of native histograms during scraping is now a separate concern controlled by Prometheus config. I see two paths forward here. We could automatically inject `scrape_native_histograms: true` into the scrape config whenever our feature gate is enabled, preserving the current behavior where enabling the feature gate is all users need to do. Alternatively, we could simply remove the line and require users to configure `scrape_native_histograms: true` in their Prometheus scrape config themselves, which is more explicit but adds an extra configuration step. What do you think? I'm also wondering if this constitutes a breaking change that should be called out in the changelog. Would appreciate any thoughts! --------- Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
Which issue(s) does the PR fix:
Does this PR introduce a user-facing change?