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

feat(pyroscope.write): Add labels validation #2918

Merged
merged 3 commits into from
Mar 12, 2025

Conversation

marcsanmi
Copy link
Contributor

@marcsanmi marcsanmi commented Mar 6, 2025

PR Description

This PR adds validation for labels in the pyroscope.write component to ensure compatibility with Pyroscope's validation requirements. The validation:

  • Checks for duplicate labels
  • Validates label names & values according to Pyroscope's standards

Which issue(s) this PR fixes

https://github.com/grafana/pyroscope-squad/issues/357

Notes to the Reviewer

  • I don't check for service_name label existence. As talked, as far as using the receive_http, this should come for free with our ensureServiceName function and also validated at pyroscope side.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

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 adding this.

LGTM

@marcsanmi marcsanmi force-pushed the marcsanmi/add-labels-validation branch from 874d4db to 4879713 Compare March 10, 2025 15:35
@marcsanmi marcsanmi requested a review from simonswine March 10, 2025 15:36
Comment on lines 500 to 506
// Skip label name validation for pyroscope reserved labels
if l.Name == pyroscope.LabelName {
lastLabelName = l.Name
continue
}

// Validate label name
if err := labelset.ValidateLabelName(l.Name); err != nil {
return fmt.Errorf("invalid label name: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Find that easier to read and less error prone

Suggested change
// Skip label name validation for pyroscope reserved labels
if l.Name == pyroscope.LabelName {
lastLabelName = l.Name
continue
}
// Validate label name
if err := labelset.ValidateLabelName(l.Name); err != nil {
return fmt.Errorf("invalid label name: %w", err)
}
// Skip label name validation for pyroscope reserved labels
if l.Name != pyroscope.LabelName {
// Validate label name
if err := labelset.ValidateLabelName(l.Name); err != nil {
return fmt.Errorf("invalid label name: %w", err)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If you disagree, just ignore

@marcsanmi marcsanmi force-pushed the marcsanmi/add-labels-validation branch from 4879713 to 9ecf5e0 Compare March 11, 2025 17:00
@marcsanmi marcsanmi requested a review from simonswine March 11, 2025 17:00
@marcsanmi marcsanmi force-pushed the marcsanmi/add-labels-validation branch from 9ecf5e0 to fbee43d Compare March 12, 2025 11:09
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.

LGTM

@marcsanmi marcsanmi merged commit 8625747 into main Mar 12, 2025
33 checks passed
@marcsanmi marcsanmi deleted the marcsanmi/add-labels-validation branch March 12, 2025 11:52
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.

2 participants