Skip to content

Commit

Permalink
Make sure NotSelector selects series with label not set
Browse files Browse the repository at this point in the history
Fixes prometheus#3575

Comes with a slight performance penalty which is better off
addressed in tsdb

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
  • Loading branch information
gouthamve committed Dec 13, 2017
1 parent 9532c2c commit 67a8b76
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 6 deletions.
4 changes: 2 additions & 2 deletions storage/fanout.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func (q *mergeQuerier) Select(matchers ...*labels.Matcher) (SeriesSet, error) {
}
seriesSets = append(seriesSets, set)
}
return NewMergeSeriesSet(seriesSets), nil
return NewMergeSeriesSet(seriesSets...), nil
}

// LabelValues returns all potential values for a label name.
Expand Down Expand Up @@ -302,7 +302,7 @@ type mergeSeriesSet struct {

// NewMergeSeriesSet returns a new series set that merges (deduplicates)
// series returned by the input series sets when iterating.
func NewMergeSeriesSet(sets []SeriesSet) SeriesSet {
func NewMergeSeriesSet(sets ...SeriesSet) SeriesSet {
// Sets need to be pre-advanced, so we can introspect the label of the
// series under the cursor.
var h seriesSetHeap
Expand Down
2 changes: 1 addition & 1 deletion storage/fanout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func TestMergeSeriesSet(t *testing.T) {
),
},
} {
merged := NewMergeSeriesSet(tc.input)
merged := NewMergeSeriesSet(tc.input...)
for merged.Next() {
require.True(t, tc.expected.Next())
actualSeries := merged.At()
Expand Down
27 changes: 26 additions & 1 deletion storage/tsdb/tsdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,39 @@ type querier struct {

func (q querier) Select(oms ...*labels.Matcher) (storage.SeriesSet, error) {
ms := make([]tsdbLabels.Matcher, 0, len(oms))
var err error
notSeen := false

for _, om := range oms {
for i, om := range oms {
ms = append(ms, convertMatcher(om))

// See: https://github.com/prometheus/prometheus/issues/3575
if om.Type == labels.MatchNotRegexp || om.Type == labels.MatchNotEqual {
if notSeen == false {
notSeen = true

oms[i], err = labels.NewMatcher(labels.MatchEqual, om.Name, "")
if err != nil {
return nil, err
}
}
}
}

set, err := q.q.Select(ms...)
if err != nil {
return nil, err
}

if notSeen {
lMissingSet, err := q.Select(oms...)
if err != nil {
return nil, err
}

return storage.NewMergeSeriesSet(lMissingSet, seriesSet{set: set}), nil
}

return seriesSet{set: set}, nil
}

Expand Down
2 changes: 1 addition & 1 deletion web/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ func (api *API) series(r *http.Request) (interface{}, *apiError) {
sets = append(sets, s)
}

set := storage.NewMergeSeriesSet(sets)
set := storage.NewMergeSeriesSet(sets...)
metrics := []labels.Labels{}
for set.Next() {
metrics = append(metrics, set.At().Labels())
Expand Down
2 changes: 1 addition & 1 deletion web/federate.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (h *Handler) federation(w http.ResponseWriter, req *http.Request) {
sets = append(sets, s)
}

set := storage.NewMergeSeriesSet(sets)
set := storage.NewMergeSeriesSet(sets...)
for set.Next() {
s := set.At()

Expand Down

0 comments on commit 67a8b76

Please sign in to comment.