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

distributor: remove labels with empty values #2439

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

bboreham
Copy link
Contributor

Empty labels are stripped when the series reaches TSDB, so we should remove them earlier to keep things consistent.

Checklist

  • Tests updated
  • NA Documentation added - this is not user-visible; we are just stripping them earlier.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

CHANGELOG.md Outdated
@@ -18,6 +18,7 @@
* [ENHANCEMENT] Object storage can now be configured for all components using the `common` YAML config option key (or `-common.storage.*` CLI flags). #2330
* [ENHANCEMENT] Go: updated to go 1.18.4. #2400
* [ENHANCEMENT] Store-gateway, listblocks: list of blocks now includes stats from `meta.json` file: number of series, samples and chunks. #2425
* [ENHANCEMENT] Distributor: remove labels with empty values #2439
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [ENHANCEMENT] Distributor: remove labels with empty values #2439
* [ENHANCEMENT] Distributor: remove labels with empty values. #2439

@@ -506,6 +506,15 @@ func removeLabel(labelName string, labels *[]mimirpb.LabelAdapter) {
}
}

// Remove labels with value=="" from a slice of LabelPairs, updating the slice in-place.
func removeEmptyLabelValues(labels *[]mimirpb.LabelAdapter) {
for i := len(*labels) - 1; i >= 0; i-- {
Copy link
Member

Choose a reason for hiding this comment

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

Could there be any performance difference when processing labels from the beginning vs from the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect the idea was if we have a, b, c, d, e and we're going to remove b and d, then going forwards means we copy d before removing it, whereas going backwards avoids that.

Also, going forward you need to not increment i if you removed an item.

@bboreham bboreham requested a review from a team as a code owner September 22, 2022 17:40
@pracucci
Copy link
Collaborator

pracucci commented Oct 7, 2022

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Empty labels are stripped when the series reaches TSDB, so we should
remove them earlier to keep things consistent.
@bboreham
Copy link
Contributor Author

bboreham commented Dec 6, 2022

Rebased against latest main; reworded the test to use labels.FromStrings().

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I hope you don't mind I've committed few more test cases.

@pracucci pracucci enabled auto-merge (squash) December 6, 2022 10:20
@pracucci pracucci merged commit 0384fac into main Dec 6, 2022
@pracucci pracucci deleted the dist-remove-empty-labels branch December 6, 2022 10:35
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
* distributor: remove labels with empty values

Empty labels are stripped when the series reaches TSDB, so we should
remove them earlier to keep things consistent.

* Added more test cases

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants