[Fix] Fix prometheus label value is not valid utf 8 cause api timeout #7128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a3dcfd5 to
df9e1db
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR resolves issue #2944 by replacing calls to WithLabelValues with GetMetricWithLabelValues to avoid a panic when receiving invalid UTF-8 label values. It also adds test cases to verify that counters, gauges, timers, and histograms return their corresponding null metrics when encountering these invalid labels.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/metrics/prometheus/factory_test.go | Adds tests to ensure invalid UTF-8 labels result in null metrics. |
| internal/metrics/prometheus/factory.go | Replaces WithLabelValues with GetMetricWithLabelValues and adds logging for error cases. |
8956652 to
26d5f0a
Compare
yurishkuro
reviewed
May 17, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7128 +/- ##
==========================================
- Coverage 96.20% 96.20% -0.01%
==========================================
Files 362 363 +1
Lines 21853 21877 +24
==========================================
+ Hits 21024 21046 +22
- Misses 620 621 +1
- Partials 209 210 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d396b9f to
eaef979
Compare
yurishkuro
reviewed
May 20, 2025
180487d to
01e949c
Compare
yurishkuro
reviewed
May 22, 2025
7c2ab64 to
02f2274
Compare
yurishkuro
reviewed
May 25, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 25, 2025
## Which problem is this PR solving? - The need for "sanitizer" concept some up in other scenarios, e.g. #7128, but currently it was hidden under jaeger binary internal. ## Description of the changes - Move it instead to internal/jptrace/sanitizer, which is a reasonably logical place considering that it affects the data model ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
…ometheus library Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
… temporary variable, because it is used 2 times Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
… ones Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
feb7d25 to
0a64ce6
Compare
…ackage Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
yurishkuro
approved these changes
May 26, 2025
This was referenced Jul 23, 2025
Closed
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 25, 2025
## Which problem is this PR solving? - Resolves part of #7261 ## Description of the changes - Files identified by deadcode tool: `internal/jptrace/utf8.go` - `SanitizeUTF8` function unreachable - When introduced: PR #7128 - When unused: the pr that introduced it never actually used the function - The fix was implemented by switching from `WithLabelValues()` to `GetMetricWithLabelValues()` in the prometheus factory, the `SanitizeUTF8` function was added but never used. - Files to remove: - `internal/jptrace/utf8.go` - `internal/jptrace/utf8_test.go` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` Signed-off-by: Parship Chowdhury <i.am.parship@gmail.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
hist.GetMetricWithLabelValuesinstead ofhist.WithLabelValuesto avoid the panic 'Label value is not valid UTF-8' and receive instead of that empty counter/gauge/timer/histogram.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test