Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

Correctly handle empty labels. #594

Merged
merged 1 commit into from
May 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,29 @@ func TestDBAppenderAddRef(t *testing.T) {
}, res)
}

func TestAppendEmptyLabelsIgnored(t *testing.T) {
db, delete := openTestDB(t, nil)
defer func() {
testutil.Ok(t, db.Close())
delete()
}()

app1 := db.Appender()

ref1, err := app1.Add(labels.FromStrings("a", "b"), 123, 0)
testutil.Ok(t, err)

// Construct labels manually so there is an empty label.
ref2, err := app1.Add(labels.Labels{labels.Label{"a", "b"}, labels.Label{"c", ""}}, 124, 0)
testutil.Ok(t, err)

// Should be the same series.
testutil.Equals(t, ref1, ref2)

err = app1.Commit()
testutil.Ok(t, err)
}

func TestDeleteSimple(t *testing.T) {
numSamples := int64(10)

Expand Down Expand Up @@ -1580,26 +1603,26 @@ func TestDB_LabelNames(t *testing.T) {
}{
{
sampleLabels1: [][2]string{
[2]string{"name1", ""},
[2]string{"name3", ""},
[2]string{"name2", ""},
[2]string{"name1", "1"},
[2]string{"name3", "3"},
[2]string{"name2", "2"},
},
sampleLabels2: [][2]string{
[2]string{"name4", ""},
[2]string{"name1", ""},
[2]string{"name4", "4"},
[2]string{"name1", "1"},
},
exp1: []string{"name1", "name2", "name3"},
exp2: []string{"name1", "name2", "name3", "name4"},
},
{
sampleLabels1: [][2]string{
[2]string{"name2", ""},
[2]string{"name1", ""},
[2]string{"name2", ""},
[2]string{"name2", "2"},
[2]string{"name1", "1"},
[2]string{"name2", "2"},
},
sampleLabels2: [][2]string{
[2]string{"name6", ""},
[2]string{"name0", ""},
[2]string{"name6", "6"},
[2]string{"name0", "0"},
},
exp1: []string{"name1", "name2"},
exp2: []string{"name0", "name1", "name2", "name6"},
Expand Down
3 changes: 3 additions & 0 deletions head.go
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,9 @@ func (a *headAppender) Add(lset labels.Labels, t int64, v float64) (uint64, erro
return 0, ErrOutOfBounds
}

// Ensure no empty labels have gotten through.
lset = lset.WithoutEmpty()

s, created := a.head.getOrCreate(lset.Hash(), lset)
if created {
a.series = append(a.series, RefSeries{
Expand Down
25 changes: 23 additions & 2 deletions labels/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,23 @@ func (ls Labels) Map() map[string]string {
return m
}

// WithoutEmpty returns the labelset without empty labels.
// May return the same labelset.
func (ls Labels) WithoutEmpty() Labels {
Copy link
Contributor

@bwplotka bwplotka Apr 26, 2019

Choose a reason for hiding this comment

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

Maybe my personal opinion, but I would say just function is better here as it has to be done only once at certain code logic.

Having this as Labels method indicates you need to do it a lot... but we don't want unnecceary run through all labels if not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this turns out to be a bigger problem than I think, we'll need to sprinkle this across the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly either way.

for _, v := range ls {
if v.Value == "" {
els := make(Labels, 0, len(ls)-1)
for _, v := range ls {
if v.Value != "" {
els = append(els, v)
}
}
return els
}
}
return ls
}

// New returns a sorted Labels from the given labels.
// The caller has to guarantee that all label names are unique.
func New(ls ...Label) Labels {
Expand All @@ -119,7 +136,9 @@ func New(ls ...Label) Labels {
func FromMap(m map[string]string) Labels {
l := make(Labels, 0, len(m))
for k, v := range m {
l = append(l, Label{Name: k, Value: v})
if v != "" {
l = append(l, Label{Name: k, Value: v})
}
}
sort.Sort(l)

Expand All @@ -133,7 +152,9 @@ func FromStrings(ss ...string) Labels {
}
var res Labels
for i := 0; i < len(ss); i += 2 {
res = append(res, Label{Name: ss[i], Value: ss[i+1]})
if ss[i+1] != "" {
res = append(res, Label{Name: ss[i], Value: ss[i+1]})
}
}

sort.Sort(res)
Expand Down