bugfix: Fix otlp translator for foreign characters#15249
Merged
ArthurSens merged 2 commits intomainfrom Oct 31, 2024
Merged
Conversation
ArthurSens
changed the title
ArthurSens
force-pushed
the
otlp-fix-sanitiza
branch
from
65cfff3 to
a4b1487
Compare
ArthurSens
force-pushed
the
otlp-fix-sanitiza
branch
2 times, most recently
from
94bccd7 to
8df09f3
Compare
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
ArthurSens
force-pushed
the
otlp-fix-sanitiza
branch
from
8df09f3 to
ea06f1a
Compare
ArthurSens
changed the title
ArthurSens
changed the title
ArthurSens
changed the title
Member
Author
|
Lol apologies for being so indecisive if I'm fixing colons in this PR or not 😅 |
dashpole
approved these changes
Oct 31, 2024
Contributor
|
Can you add a changelog entry for the changes in this PR? I still lack the context as to why the changes are necessary, and what the consequences are. I think the PR description should have contained all the context about the changes, instead of back-referencing a previous PR where they were not the main topic. |
aknuds1
reviewed
Nov 2, 2024
Contributor
aknuds1
left a comment
aknuds1
left a comment
There was a problem hiding this comment.
I suggest fixing some test typos.
| {"label:with:colons", "label_with_colons"}, // Without UTF-8 support, colons are only allowed in metric names | ||
| {"LabelWithCapitalLetters", "LabelWithCapitalLetters"}, | ||
| {"label!with&special$chars)", "label_with_special_chars_"}, | ||
| {"label_with_foreign_characteres_字符", "label_with_foreign_characteres___"}, |
Contributor
There was a problem hiding this comment.
Suggested change
| {"label_with_foreign_characteres_字符", "label_with_foreign_characteres___"}, | |
| {"label_with_foreign_characters_字符", "label_with_foreign_characters___"}, |
| require.Equal(t, ":foo::bar", BuildCompliantName(createCounter(":foo::bar", ""), "", false)) | ||
| require.Equal(t, "foo_bar", BuildCompliantName(createGauge("foo.bar", "1"), "", false)) | ||
| require.Equal(t, "system_io", BuildCompliantName(createCounter("system.io", "foo/bar"), "", false)) | ||
| require.Equal(t, "metric_with___foreign_characteres", BuildCompliantName(createCounter("metric_with_字符_foreign_characteres", ""), "", false)) |
Contributor
There was a problem hiding this comment.
Suggested change
| require.Equal(t, "metric_with___foreign_characteres", BuildCompliantName(createCounter("metric_with_字符_foreign_characteres", ""), "", false)) | |
| require.Equal(t, "metric_with___foreign_characters", BuildCompliantName(createCounter("metric_with_字符_foreign_characters", ""), "", false)) |
Merged
julienduchesne
pushed a commit
to julienduchesne/prometheus
that referenced
this pull request
Dec 13, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
The fix was originally proposed in #14974, but we're moving it to a separate PR to keep the scope of changes smaller.
In the end, I've also decided to go in a different direction than proposed in #14974 for this fix. I've looked up the unicode table and just choose to use IsUpper and IsLower instead of the whole Letter category.