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

fix: normalize label name to follow prometheus spec #4264

Merged
merged 4 commits into from
Dec 28, 2023

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Dec 21, 2023

Summary

See the metrics labels section of https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels. The valid label name doesn't allow any special characters other than _. This PR normalizes the label name so that alertmanager doesn't reject the alert sending.

Maybe TODO: SigNoz/alertmanager#22

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the bug Something isn't working label Dec 21, 2023
@srikanthccv srikanthccv marked this pull request as ready for review December 26, 2023 02:49
@ankitnayan
Copy link
Collaborator

Correct me if I am missing anything. There are 3 ways to solve this issue

  1. Reject labels which do not comply with the above regex in the UI itself. But I guess, users do not control the value of labels and hence we cannot use this method? And if so, this would be huge problem in prometheus as users will have to be very cautious about the values they put in.
  2. Convert non-supported chars to _ as done in this PR
  3. Fix alertmanager to support the other chars. But I guess, the maintainers at Prometheus must have placed that restriction due to same reason. We should find that out before fixing at alertmanager

@srikanthccv
Copy link
Member Author

srikanthccv commented Dec 28, 2023

Reject labels which do not comply with the above regex in the UI itself. But I guess, users do not control the value of labels and hence we cannot use this method? And if so, this would be huge problem in prometheus as users will have to be very cautious about the values they put in.

Convert non-supported chars to _ as done in this PR

We are only concerned with the the label key not the value. The value can be whatever users set. We are only normalizing the label key string. We cannot reject in the UI itself because users want to set alerts on logs/traces which currently have attribute keys/names with dots in them.

https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels: Label values may contain any Unicode characters.

Fix alertmanager to support the other chars. But I guess, the maintainers at Prometheus must have placed that restriction due to same reason. We should find that out before fixing at alertmanager

Yes, I have opened a separate issue for that. The delivery of the alert is more important now. Eventually, we need to find a way to solve this issue.

@ankitnayan ankitnayan self-requested a review December 28, 2023 14:30
@srikanthccv srikanthccv merged commit 9230f24 into develop Dec 28, 2023
11 checks passed
@srikanthccv srikanthccv deleted the normalize-label-key branch December 28, 2023 14:52
@ankitnayan
Copy link
Collaborator

@srikanthccv will the url to the alert work fine?

@srikanthccv
Copy link
Member Author

Yes, we are not changing anything in the URL. The URL is prepared (as of today) based on the query range payload not the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants