Skip to content

Handle error gracefully for the desymbolizeLabels function in prompb/io/prometheus/write/v2/symbols.go#17160

Merged
cstyan merged 6 commits intoprometheus:mainfrom
pipiland2612:fix_panic
Sep 8, 2025
Merged

Handle error gracefully for the desymbolizeLabels function in prompb/io/prometheus/write/v2/symbols.go#17160
cstyan merged 6 commits intoprometheus:mainfrom
pipiland2612:fix_panic

Conversation

@pipiland2612
Copy link
Contributor

Which issue(s) does the PR fix:

Fixes #17149

Changes

Does this PR introduce a user-facing change?

NONE

pipiland added 2 commits September 8, 2025 15:31
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
fix
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing and clean! 💪🏽

Want to fix another potential panic while we are at this code?

@pipiland2612
Copy link
Contributor Author

pipiland2612 commented Sep 8, 2025

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>
@pipiland2612 pipiland2612 reopened this Sep 8, 2025
@pipiland2612
Copy link
Contributor Author

pipiland2612 commented Sep 8, 2025

Retriggering the CI ( I don't know there is some test that is non deterministic, sometimes it pass sometimes is doesn't)

@pipiland2612 pipiland2612 reopened this Sep 8, 2025
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing! One more thing only, otherwise LGTM!

fix
Signed-off-by: pipiland <user123@Minhs-Macbook.local>
@cstyan
Copy link
Member

cstyan commented Sep 8, 2025

@pipiland2612 the tests need to be updated to look for the new string formats

Signed-off-by: pipiland <user123@Minhs-Macbook.local>
@pipiland2612
Copy link
Contributor Author

@pipiland2612 the tests need to be updated to look for the new string formats

Yup, thanks for noticing me. Fixed at dcce5b5

Copy link
Member

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

LGTM, like you said we just have an empty labelset on error so I think printing the refs is fine

@cstyan cstyan merged commit 0fc2547 into prometheus:main Sep 8, 2025
28 checks passed
@pipiland2612 pipiland2612 deleted the fix_panic branch September 8, 2025 20:05
Naman-B-Parlecha added a commit to Naman-B-Parlecha/prometheus that referenced this pull request Sep 17, 2025
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>
dmitryax added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Oct 31, 2025
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
jelly-afk pushed a commit to jelly-afk/opentelemetry-collector-contrib that referenced this pull request Nov 6, 2025
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
dyl10s pushed a commit to dyl10s/opentelemetry-collector-contrib that referenced this pull request Nov 21, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remote Write 2.0 panics with an incorrect symbols table

3 participants