Skip to content

utf8: Remove support for legacy global validation setting#16080

Merged
ywwg merged 1 commit intomainfrom
owilliams/escapeconfig-00-cleanup
Mar 13, 2025
Merged

utf8: Remove support for legacy global validation setting#16080
ywwg merged 1 commit intomainfrom
owilliams/escapeconfig-00-cleanup

Conversation

@ywwg
Copy link
Copy Markdown
Member

@ywwg ywwg commented Feb 26, 2025 •
edited
Loading

Global and Data Source configurations can still specify legacy mode, but Prometheus now requires that the overall validation mode be set to UTF-8.

ywwg
ywwg commented Feb 26, 2025
View reviewed changes
@ywwg ywwg changed the title scraper: Remove support for global legacy validation mode scraper: Remove support for global legacy validation setting Feb 26, 2025
@ywwg ywwg marked this pull request as ready for review February 26, 2025 17:24
@ywwg ywwg force-pushed the owilliams/escapeconfig-00-cleanup branch from f628748 to bb1fe65 Compare February 26, 2025 17:24
@ywwg ywwg mentioned this pull request Feb 26, 2025
Merged
bwplotka
bwplotka approved these changes Feb 28, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Much cleaner, thanks!

@ywwg ywwg force-pushed the owilliams/escapeconfig-00-cleanup branch from e9e16f9 to db82701 Compare March 12, 2025 18:25
@ywwg ywwg requested a review from bwplotka March 12, 2025 18:26
bwplotka
bwplotka approved these changes Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Thanks! Lgtm

Nit: Consider changing PR title as it's bit wider that just scraper (or/and) commit title.

@ywwg ywwg changed the title scraper: Remove support for global legacy validation setting deps: Remove support for legacy global validation setting Mar 13, 2025
@ywwg
utf8: Remove support for legacy global validation setting
94b43c5 
Global and Data Source configurations can specify legacy mode, but Prometheus now requires that the overall validation mode be set to UTF-8

Signed-off-by: Owen Williams <[email protected]>
@ywwg ywwg changed the title deps: Remove support for legacy global validation setting utf8: Remove support for legacy global validation setting Mar 13, 2025
@ywwg ywwg force-pushed the owilliams/escapeconfig-00-cleanup branch from db82701 to 94b43c5 Compare March 13, 2025 14:48
@bwplotka
Copy link
Copy Markdown
Member

bwplotka commented Mar 13, 2025 

=== NAME  TestInvalidFileUpdate
    file_test.go:414: 
        	Error Trace:	D:/a/prometheus/prometheus/discovery/file/file_test.go:107
        	            				D:/a/prometheus/prometheus/discovery/file/file_test.go:414
        	Error:      	Received unexpected error:
        	            	rename C:\Users\RUNNER~1\AppData\Local\Temp\TestInvalidFileUpdate1329895897\001\1399140205 C:\Users\RUNNER~1\AppData\Local\Temp\TestInvalidFileUpdate1329895897\001\valid.yml: Access is denied.
        	Test:       	TestInvalidFileUpdate
--- FAIL: TestInvalidFileUpdate (0.11s)
=== CONT  TestFileUpdate
=== NAME  TestRemoveFile
    file_test.go:482: update received after 102.0034ms
--- PASS: TestRemoveFile (0.21s)
=== NAME  TestNoopFileUpdate
    file_test.go:380: update received after 106.6192ms
--- PASS: TestNoopFileUpdate (0.21s)
=== NAME  TestFileUpdate
    file_test.go:397: 
        	Error Trace:	D:/a/prometheus/prometheus/discovery/file/file_test.go:90
        	            				D:/a/prometheus/prometheus/discovery/file/file_test.go:397
        	Error:      	Received unexpected error:
        	            	rename C:\Users\RUNNER~1\AppData\Local\Temp\TestFileUpdate742520923\001\2492825128 C:\Users\RUNNER~1\AppData\Local\Temp\TestFileUpdate742520923\001\valid.yml: Access is denied.
        	Test:       	TestFileUpdate
--- FAIL: TestFileUpdate (0.10s)

Windows is consistently failing so far...

@ywwg
Copy link
Copy Markdown
Member Author

ywwg commented Mar 13, 2025

Windows is consistently failing so far...

how did you find the error? the log is humongous and there are 100 instances of the word "error" and I couldn't find this

@ywwg
Copy link
Copy Markdown
Member Author

ywwg commented Mar 13, 2025

windows error appears on other PRs, merging anyway.

@ywwg
Copy link
Copy Markdown
Member Author

ywwg commented Mar 13, 2025

mmm after I check that the docs are still accurate

@ywwg ywwg merged commit 0bfbd46 into main Mar 13, 2025
43 of 45 checks passed
@ywwg ywwg deleted the owilliams/escapeconfig-00-cleanup branch March 13, 2025 18:21
@ywwg ywwg added the area/utf8 Issues pertaining to UTF-8 support label Mar 13, 2025
@charleskorn charleskorn mentioned this pull request Mar 21, 2025
Merged
ywwg added a commit that referenced this pull request Mar 26, 2025
@ywwg
scrape: Add config option for escaping scheme request. (#16066)
6566c5a 
The new metric_name_escaping_scheme config option works in parallel with metric_name_validation_scheme and controls which escaping scheme is requested when scraping. When not specified, the scheme will request underscores if the validation scheme is set to legacy, and will request allow-utf-8 when the validation scheme is set to utf8. This setting allows users to allow utf8 names if they like, but explicitly request an escaping scheme rather than UTF-8.

Fixes #16034

Built on #16080

Signed-off-by: Owen Williams <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwplotka bwplotka bwplotka approved these changes

@dgl dgl Awaiting requested review from dgl dgl is a code owner

@roidelapluie roidelapluie Awaiting requested review from roidelapluie roidelapluie is a code owner

@cstyan cstyan Awaiting requested review from cstyan cstyan is a code owner

@tomwilkie tomwilkie Awaiting requested review from tomwilkie tomwilkie is a code owner

Assignees

No one assigned

Labels

area/utf8 Issues pertaining to UTF-8 support

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ywwg @bwplotka