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: infinite loop in index writer #3356

Merged
merged 2 commits into from
Jun 18, 2024
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
36 changes: 36 additions & 0 deletions pkg/model/labels_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,42 @@ func TestLabelsUnique(t *testing.T) {
}
}

func Test_LabelsBuilder_Unique(t *testing.T) {
tests := []struct {
name string
input Labels
add Labels
expected Labels
}{
{
name: "duplicates in input are preserved",
input: Labels{
{Name: "unique", Value: "yes"},
{Name: "unique", Value: "no"},
},
add: Labels{
{Name: "foo", Value: "bar"},
{Name: "foo", Value: "baz"},
},
expected: Labels{
{Name: "foo", Value: "baz"},
{Name: "unique", Value: "yes"},
{Name: "unique", Value: "no"},
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
builder := NewLabelsBuilder(test.input)
for _, l := range test.add {
builder.Set(l.Name, l.Value)
}
assert.Equal(t, test.expected, builder.Labels())
})
}
}

Comment on lines +71 to +106
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not see any practical sense in fixing LabelsBuilder we borrowed from prometheus. Instead, in our case, a reusable map would work way better (and I'm 99.(9)% sure, more efficiently).

To be addressed in a separate PR.

image

func TestLabels_SessionID_Order(t *testing.T) {
input := []Labels{
{
Expand Down
1 change: 1 addition & 0 deletions pkg/phlaredb/labels/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func CreateProfileLabels(enforceOrder bool, p *profilev1.Profile, externalLabels
} else {
sort.Sort(lbs)
}
lbs = lbs.Unique()
Copy link
Collaborator Author

@kolesnikovae kolesnikovae Jun 14, 2024

Choose a reason for hiding this comment

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

Just in case: Unique works fine with LabelsEnforcedOrder

profilesLabels[pos] = lbs
seriesRefs[pos] = model.Fingerprint(lbs.Hash())

Expand Down
29 changes: 29 additions & 0 deletions pkg/phlaredb/phlaredb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,35 @@ func TestMergeProfilesStacktraces(t *testing.T) {
})
}

// See https://github.com/grafana/pyroscope/pull/3356
func Test_HeadFlush_DuplicateLabels(t *testing.T) {
defer goleak.VerifyNone(t, goleak.IgnoreCurrent())

// ingest some sample data
var (
ctx = testContext(t)
testDir = contextDataDir(ctx)
end = time.Unix(0, int64(time.Hour))
start = end.Add(-time.Minute)
step = 15 * time.Second
)

db, err := New(ctx, Config{
DataPath: testDir,
MaxBlockDuration: time.Duration(100000) * time.Minute,
}, NoLimit, ctx.localBucketClient)
require.NoError(t, err)
defer func() {
require.NoError(t, db.Close())
}()

ingestProfiles(t, db, cpuProfileGenerator, start.UnixNano(), end.UnixNano(), step,
&typesv1.LabelPair{Name: "namespace", Value: "my-namespace"},
&typesv1.LabelPair{Name: "pod", Value: "my-pod"},
&typesv1.LabelPair{Name: "pod", Value: "not-my-pod"},
)
}

func TestMergeProfilesPprof(t *testing.T) {
defer goleak.VerifyNone(t, goleak.IgnoreCurrent())

Expand Down
6 changes: 4 additions & 2 deletions pkg/phlaredb/tsdb/index/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,10 @@ func (w *Writer) writePostingsToTmpFiles() error {
// using more memory than a single label name can.
for len(names) > 0 {
if w.labelNames[names[0]]+c > maxPostings {
break
if c > 0 {
break
}
return fmt.Errorf("corruption detected when writing postings to index: label %q has %d uses, but maxPostings is %d", names[0], w.labelNames[names[0]], maxPostings)
}
batchNames = append(batchNames, names[0])
c += w.labelNames[names[0]]
Expand Down Expand Up @@ -1016,7 +1019,6 @@ func (w *Writer) writePostingsToTmpFiles() error {
return w.ctx.Err()
default:
}

}
return nil
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/phlaredb/tsdb/index/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -584,3 +584,18 @@ func TestDecoder_Postings_WrongInput(t *testing.T) {
_, _, err := (&Decoder{}).Postings([]byte("the cake is a lie"))
require.Error(t, err)
}

func TestWriter_ShouldReturnErrorOnSeriesWithDuplicatedLabelNames(t *testing.T) {
w, err := NewWriter(context.Background(), filepath.Join(t.TempDir(), "index"))
require.NoError(t, err)

require.NoError(t, w.AddSymbol("__name__"))
require.NoError(t, w.AddSymbol("metric_1"))
require.NoError(t, w.AddSymbol("metric_2"))

require.NoError(t, w.AddSeries(0, phlaremodel.LabelsFromStrings("__name__", "metric_1", "__name__", "metric_2"), 0))

err = w.Close()
require.Error(t, err)
require.ErrorContains(t, err, "corruption detected when writing postings to index")
}
Loading