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.

Sign up for GitHub

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

Jump to bottom

Fix potential race with LabelPairs #511

Closed
wants to merge 2 commits into from
Closed

Fix potential race with LabelPairs #511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
029638f
Fix potential race with LabelPairs
douglas-reid Dec 5, 2018
cc2c5fb
use stable sort
douglas-reid Dec 6, 2018
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
2 changes: 1 addition & 1 deletion prometheus/histogram.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ func (h *histogram) Write(out *dto.Metric) error {

his.Bucket = buckets
out.Histogram = his
out.Label = h.labelPairs
out.Label = copyLabelPairs(h.labelPairs)

// Finally add all the cold counts to the new hot counts and reset the cold counts.
atomic.AddUint64(&hotCounts.count, count)
Expand Down
2 changes: 1 addition & 1 deletion prometheus/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,7 +872,7 @@ func checkMetricConsistency(
h = hashAddByte(h, separatorByte)
// Make sure label pairs are sorted. We depend on it for the consistency
// check.
sort.Sort(labelPairSorter(dtoMetric.Label))
sort.Stable(labelPairSorter(dtoMetric.Label))
for _, lp := range dtoMetric.Label {
h = hashAdd(h, lp.GetName())
h = hashAddByte(h, separatorByte)
Expand Down
19 changes: 16 additions & 3 deletions prometheus/registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ func TestHistogramVecRegisterGatherConcurrency(t *testing.T) {
Help: "This helps testing.",
ConstLabels: prometheus.Labels{"foo": "bar"},
},
[]string{"one", "two", "three"},
[]string{"one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten", "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen"},
Copy link

Choose a reason for hiding this comment

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

Do we know the sweet spot that this happens? Or, even better, the underlying reason why more makes this break?

Copy link
Author

Choose a reason for hiding this comment

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

@bobbytables i have not yet identified the exact scenario under which things get wonky. My guess is the sorting of the labelpairs can be racy. This PR is really just an attempt to highlight the issue and hopefully lead to further discovery, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I think the reason is here: https://golang.org/src/sort/sort.go?s=5414:5439#L184

I.e. the sort algorithm changes with more than 12 elements.

)
labelValues = []string{"a", "b", "c", "alpha", "beta", "gamma", "aleph", "beth", "gimel"}
quit = make(chan struct{})
Expand All @@ -810,6 +810,19 @@ func TestHistogramVecRegisterGatherConcurrency(t *testing.T) {
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
labelValues[rand.Intn(len(labelValues))],
).Observe(obs)
}
}
Expand Down Expand Up @@ -848,7 +861,7 @@ func TestHistogramVecRegisterGatherConcurrency(t *testing.T) {
if len(g) != 1 {
t.Error("Gathered unexpected number of metric families:", len(g))
}
if len(g[0].Metric[0].Label) != 4 {
if len(g[0].Metric[0].Label) != 17 {
t.Error("Gathered unexpected number of label pairs:", len(g[0].Metric[0].Label))
}
}
Expand All @@ -869,7 +882,7 @@ func TestHistogramVecRegisterGatherConcurrency(t *testing.T) {
go gather()
go observe()

time.Sleep(time.Second)
time.Sleep(2 * time.Second)
close(quit)
wg.Wait()
}
Expand Down
14 changes: 13 additions & 1 deletion prometheus/value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func populateMetric(
labelPairs []*dto.LabelPair,
m *dto.Metric,
) error {
m.Label = labelPairs
m.Label = copyLabelPairs(labelPairs)
switch t {
case CounterValue:
m.Counter = &dto.Counter{Value: proto.Float64(v)}
Expand Down Expand Up @@ -160,3 +160,15 @@ func makeLabelPairs(desc *Desc, labelValues []string) []*dto.LabelPair {
sort.Sort(labelPairSorter(labelPairs))
return labelPairs
}

func copyLabelPairs(source []*dto.LabelPair) []*dto.LabelPair {
labelPairs := make([]*dto.LabelPair, 0, len(source))
for _, pair := range source {
labelPairs = append(labelPairs, &dto.LabelPair{
Name: pair.Name,
Value: pair.Value,
})
Copy link

Choose a reason for hiding this comment

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

Do you need to append here since you created the slice with the correct length already? Why not just assign the index of the labelPairs to the current index in the loop?

Copy link
Author

Choose a reason for hiding this comment

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

This was mostly a copy-paste job from the method above, as I was aiming for consistency. I don't think there is any significant difference in the two approaches, but I don't feel strongly in either direction.

}
// shouldn't need sorting, as it should already be sorted
return labelPairs
}