Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pkg/translator/prometheus] Add option to keep UTF-8 characters #35904

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[pkg/translator/prometheus] Add option to keep UTF-8 characters #35904

wants to merge 1 commit into from

Conversation

ArthurSens
Copy link
Member

Description

Updates the translator package to allow keeping UTF-8 characters. The intended usage of this is that components that use this package would add new configuration options to allow users to keep UTF-8 characters in metric and label names.

For the components that are using this package, I'm adding false as the argument to keep the current behavior. Follow-up PRs will be opened to allow configurability.

Link to tracking issue

Related to #35459

@ArthurSens ArthurSens requested review from dashpole and a team as code owners October 21, 2024 19:41
@ArthurSens ArthurSens mentioned this pull request Oct 21, 2024
Open
@ArthurSens
Copy link
Member Author

@dashpole, I'm not sure what to do with the googlemanagedprometheusexporter 🤔, at least I don't understand how I can solve the problem in this PR.

Google's managed prometheus is doing some weird go mod replaces somewhere?

@dashpole
Copy link
Contributor

Yikes. Previously, we've had to deprecate and replace functions that are changed. I might be able to refactor the GMP code to move usage of the translator package back into contrib.

@ArthurSens
Copy link
Member Author

Yikes. Previously, we've had to deprecate and replace functions that are changed. I might be able to refactor the GMP code to move usage of the translator package back into contrib.

I could change the implementation into a new function BuildUTF8Name if you prefer, but it's weird that user of the library blocks updates like this 🤔

@dashpole dashpole mentioned this pull request Oct 31, 2024
Merged
@dashpole
Copy link
Contributor

working to unblock you: GoogleCloudPlatform/opentelemetry-operations-go#908

@dashpole dashpole mentioned this pull request Nov 1, 2024
Merged
bogdandrutu pushed a commit that referenced this pull request Nov 2, 2024
@dashpole
Update update github.com/GoogleCloudPlatform/opentelemetry-operations…
6dc39a8 
…-go to the latest release (#36132)

#### Description

Update github.com/GoogleCloudPlatform/opentelemetry-operations-go to
https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/releases/tag/v0.49.0.

This unblocks
#35904
by moving the prometheus sanitization into contrib.

@ArthurSens
@github-actions github-actions bot added the exporter/googlemanagedprometheus Google Managed Prometheus exporter label Nov 2, 2024
@ArthurSens
Copy link
Member Author

Thanks for unblocking this @dashpole ❤️, it should be ready for review

@ArthurSens
[pkg/translator/prometheus] Add option to keep UTF-8 characters
e5719ea 
Signed-off-by: Arthur Silva Sens <arthursens2005@gmail.com>
bertysentry
bertysentry reviewed Nov 4, 2024
View reviewed changes
pkg/translator/prometheus/normalize_name_test.go
require.Equal(t, "unsupported_metric_redundant_test_per_C", normalizeName(createGauge("unsupported.metric.redundant", "__test $/°C"), ""))

func TestAllowUTF8(t *testing.T) {
for _, allowUTF8 := range []bool{true, false} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the for followed by if/else. Unnecessary complexity.

	// Don't allow UTF-8 (default)
	require.Equal(t, "unsupported_metric_temperature_F", normalizeName(createGauge("unsupported.metric.temperature", "°F"), "", false))
	require.Equal(t, "unsupported_metric_weird", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", false))
	require.Equal(t, "unsupported_metric_redundant_test_per_C", normalizeName(createGauge("unsupported.metric.redundant", "__test $/°C"), "", false))

	// Allow UTF-8
	require.Equal(t, "unsupported.metric.temperature_°F", normalizeName(createGauge("unsupported.metric.temperature", "°F"), "", true))
	require.Equal(t, "unsupported.metric.weird_+=.:,!* & #", normalizeName(createGauge("unsupported.metric.weird", "+=.:,!* & #"), "", true))
	require.Equal(t, "unsupported.metric.redundant___test $_per_°C", normalizeName(createGauge("unsupported.metric.redundant", "__test $/°C"), "", true))

pkg/translator/prometheus/normalize_name_test.go
require.Equal(t, "system_cpu_utilization_ratio", normalizeName(createGauge("system.cpu.utilization", "1"), "", false))
require.Equal(t, "system_disk_operation_time_seconds_total", normalizeName(createCounter("system.disk.operation_time", "s"), "", false))
require.Equal(t, "system_cpu_load_average_15m_ratio", normalizeName(createGauge("system.cpu.load_average.15m", "1"), "", false))
require.Equal(t, "memcached_operation_hit_ratio_percent", normalizeName(createGauge("memcached.operation_hit_ratio", "%"), "", false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Edge case: what's the expected result if allowedUTF8 == true?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still translated to percent

pkg/translator/prometheus/normalize_name_test.go
require.Equal(t, "mongodbatlas_process_journaling_data_files_mebibytes", normalizeName(createGauge("mongodbatlas.process.journaling.data_files", "MiBy"), "", false))
require.Equal(t, "mongodbatlas_process_network_io_bytes_per_second", normalizeName(createGauge("mongodbatlas.process.network.io", "By/s"), "", false))
require.Equal(t, "mongodbatlas_process_oplog_rate_gibibytes_per_hour", normalizeName(createGauge("mongodbatlas.process.oplog.rate", "GiBy/h"), "", false))
require.Equal(t, "mongodbatlas_process_db_query_targeting_scanned_per_returned", normalizeName(createGauge("mongodbatlas.process.db.query_targeting.scanned_per_returned", "{scanned}/{returned}"), "", false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Edge case: what's the expected result if allowedUTF8 == true?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's translated to scanned_per_returned

pkg/translator/prometheus/normalize_name_test.go
require.Equal(t, "hello_you_2", CleanUpString("hello you 2"))
require.Equal(t, "1000", CleanUpString("$1000"))
require.Equal(t, "", CleanUpString("*+$^=)"))
for _, allowUTF8 := range []bool{true, false} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the for/if/else: unnecessary complexity

@ArthurSens
Copy link
Member Author

@bertysentry the questions around unit translation are really good ones, I haven't thought about it. This is something that wasn't questioned in the user survey and I don't know what should be the answer here.

Maybe we start by keeping the current behavior, and if needed change this in the future?

ArthurSens pushed a commit to ArthurSens/opentelemetry-collector-contrib that referenced this pull request Nov 4, 2024
@dashpole @ArthurSens
Update update github.com/GoogleCloudPlatform/opentelemetry-operations…
0a00ba3 
…-go to the latest release (open-telemetry#36132)

#### Description

Update github.com/GoogleCloudPlatform/opentelemetry-operations-go to
https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/releases/tag/v0.49.0.

This unblocks
open-telemetry#35904
by moving the prometheus sanitization into contrib.

@ArthurSens
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bertysentry bertysentry bertysentry left review comments

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

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@rapphil rapphil Awaiting requested review from rapphil

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

@gouthamve gouthamve Awaiting requested review from gouthamve

@mar4uk mar4uk Awaiting requested review from mar4uk

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

@aabmass aabmass Awaiting requested review from aabmass

@psx95 psx95 Awaiting requested review from psx95

@punya punya Awaiting requested review from punya

At least 1 approving review is required to merge this pull request.

Assignees

@fatsheep9146 fatsheep9146

Labels
exporter/googlemanagedprometheus Google Managed Prometheus exporter exporter/prometheus exporter/prometheusremotewrite pkg/translator/loki pkg/translator/prometheus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ArthurSens @dashpole @bertysentry @fatsheep9146