Handle error gracefully for the desymbolizeLabels function in prompb/io/prometheus/write/v2/symbols.go#17160
Conversation
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
bwplotka
left a comment
There was a problem hiding this comment.
Amazing and clean! 💪🏽
Want to fix another potential panic while we are at this code?
|
Yes, I also realise there is another potential panic here. Thanks for noticing me. Solved at 925bf09 |
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
|
Retriggering the CI ( I don't know there is some test that is non deterministic, sometimes it pass sometimes is doesn't) |
bwplotka
left a comment
There was a problem hiding this comment.
Amazing! One more thing only, otherwise LGTM!
|
@pipiland2612 the tests need to be updated to look for the new string formats |
Yup, thanks for noticing me. Fixed at dcce5b5 |
cstyan
left a comment
There was a problem hiding this comment.
LGTM, like you said we just have an empty labelset on error so I think printing the refs is fine
Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> feat(histogram): Refactor ConvertNHCBToClassicHistogram and add unit tests Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> feat(histogram): Add ConvertNHCBToClassic flag and refactor related types and tests Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> Replace gopkg.in/yaml.v2 with go.yaml.in/yaml/v2 (prometheus#17151) * Replace gopkg.in/yaml.v2 with go.yaml.in/yaml/v2 * Upgrade to client_golang@v1.23.2 --------- Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> OTLP to directly write to an interface which can hide storage details (prometheus#16951) * OTLP writer writes directly to appender Do not convert to Remote-Write 1.0 protocol. Convert to TSDB Appender interface instead. For downstream projects that still convert OTLP to something else (e.g. Mimir using its own RW 1.0+2.0 compatible protocol), introduce a compatibility layer between OTLP decoding and TSDB Appender. This is the CombinedAppender that hides the implementation. Name is subject to change. --------- Signed-off-by: David Ashpole <dashpole@google.com> Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: George Krajcsovits <krajorama@users.noreply.github.com> Co-authored-by: David Ashpole <dashpole@google.com> Co-authored-by: Jesus Vazquez <jesusvazquez@users.noreply.github.com> Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> fix(textparse/protobuf): metric family name corrupted by NHCB parser (prometheus#17156) * fix(textparse): implement NHCB parsing in ProtoBuf parser directly The NHCB conversion does some validation, but we can only return error from Parser.Next() not Parser.Histogram(). So the conversion needs to happen in Next(). There are 2 cases: 1. "always_scrape_classic_histograms" is enabled, in which case we convert after returning the classic series. This is to be consistent with the PromParser text parser, which collects NHCB while spitting out classic series; then returns the NHCB. 2. "always_scrape_classic_histograms" is disabled. In which case we never return the classic series. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> * refactor(textparse): skip classic series instead of adding NHCB around Do not return the first classic series from the EntryType state, switch to EntrySeries. This means we need to start the histogram field state from -3 , not -2. In EntrySeries, skip classic series if needed. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> * reuse nhcb converter Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> * test(textparse/nhcb): test corrupting metric family name NHCB parse doesn't always copy the metric name from the underlying parser. When called via HELP, UNIT, the string is directly referenced which means that the read-ahead of NHCB can corrupt it. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Handle error gracefully for the `desymbolizeLabels` function in prompb/io/prometheus/write/v2/symbols.go (prometheus#17160) Signed-off-by: pipiland <user123@Minhs-Macbook.local> --------- Signed-off-by: pipiland <user123@Minhs-Macbook.local> Co-authored-by: pipiland <user123@Minhs-Macbook.local> feat(histogram): Refactor NHCB to classic conversion with enhanced validation and update tests Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> feat(histogram): Update NHCB to classic conversion logic and remove tests Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> refactor(histogram): Update ConvertNHCBToClassicHistogram Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> test cases for new conversion Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com> refactor(histogram): Simplify label building in ConvertNHCBToClassicHistogram Signed-off-by: Naman-B-Parlecha <namanparlecha@gmail.com>
Both k8s and prometheus libraries have to be updated because they depend on each other and both have breaking changes. The following files changed to adopt to the API breaking changes: - processor/k8sattributesprocessor/internal/kube/fake_informer.go: - kubernetes/kubernetes#126387 - receiver/prometheusreceiver/metrics_receiver.go: - prometheus/prometheus#16257 - receiver/prometheusremotewritereceiver/receiver.go: - prometheus/prometheus#17160 I haven't touched any usages of deprecated k8s API to keep the changeset small. Created a separate issue for that instead #43891 Some prometheus tests are failing due to behavioral change in prometheus library. I skipped them for now and reported #43893
Both k8s and prometheus libraries have to be updated because they depend on each other and both have breaking changes. The following files changed to adopt to the API breaking changes: - processor/k8sattributesprocessor/internal/kube/fake_informer.go: - kubernetes/kubernetes#126387 - receiver/prometheusreceiver/metrics_receiver.go: - prometheus/prometheus#16257 - receiver/prometheusremotewritereceiver/receiver.go: - prometheus/prometheus#17160 I haven't touched any usages of deprecated k8s API to keep the changeset small. Created a separate issue for that instead open-telemetry#43891 Some prometheus tests are failing due to behavioral change in prometheus library. I skipped them for now and reported open-telemetry#43893
Both k8s and prometheus libraries have to be updated because they depend on each other and both have breaking changes. The following files changed to adopt to the API breaking changes: - processor/k8sattributesprocessor/internal/kube/fake_informer.go: - kubernetes/kubernetes#126387 - receiver/prometheusreceiver/metrics_receiver.go: - prometheus/prometheus#16257 - receiver/prometheusremotewritereceiver/receiver.go: - prometheus/prometheus#17160 I haven't touched any usages of deprecated k8s API to keep the changeset small. Created a separate issue for that instead open-telemetry#43891 Some prometheus tests are failing due to behavioral change in prometheus library. I skipped them for now and reported open-telemetry#43893
Which issue(s) does the PR fix:
Fixes #17149
Changes
prometheus/prompb/io/prometheus/write/v2/symbols.go
Line 76 in 913cc8f
Does this PR introduce a user-facing change?