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.

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

chore: support dots in label names #3335

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Jun 4, 2024

We're currently using prometheus data model for labels (including those that are set as pprof tags) for consistency with other products:

Labels may contain ASCII letters, numbers, as well as underscores. They must match the regex [a-zA-Z_][a-zA-Z0-9_]*.

Although we can not easily loosen the requirement, I'm not very sure if discarding profiles that violate it is the best strategy.

There is ongoing work on support for UTF-8 labels in Prometheus:
prometheus/prometheus#13095:

OTel allows UTF-8 in label names while Prometheus has a much more restrictive set. This is causing friction for users when using Prometheus. In particular, . (dot) is a very common character in OTel and we convert that to _ when converting to Prometheus. For example, service.version becomes service_version.

Until the support is fully implemented, I propose to do the same in Pyroscope – convert . to _ in label names to make interoperability easier. Theoretically, we could replace any disallowed characters, but that leads to ambiguity in certain cases and may confuse users.

@kolesnikovae kolesnikovae force-pushed the chore/sanitize_label_names branch from b7fcca7 to 6e8cc7d Compare June 4, 2024 09:02
@kolesnikovae kolesnikovae changed the title chore: sanitize label names chore: support dots in label names Jun 4, 2024
@kolesnikovae kolesnikovae marked this pull request as ready for review June 4, 2024 09:16
@kolesnikovae kolesnikovae requested a review from a team as a code owner June 4, 2024 09:16
Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

lgtm

pkg/validation/validate_test.go Show resolved Hide resolved
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thanks for fixing/looking into this. LGTM

A small concern not necessarily blocking, maybe we slightly adapt the output, or that we modified label names with . to _, if we did. Just wonder about your opinion.

pkg/validation/validate.go Outdated Show resolved Hide resolved
@kolesnikovae kolesnikovae force-pushed the chore/sanitize_label_names branch from 37a9380 to a35cb8c Compare June 4, 2024 11:10
@kolesnikovae kolesnikovae merged commit e55d2de into main Jun 4, 2024
16 checks passed
@kolesnikovae kolesnikovae deleted the chore/sanitize_label_names branch June 4, 2024 11:20
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.

3 participants