Confusing comments in silence.go

[grobinson-grafana]

What did you do?

I was looking at the code in silence.go and – unless I'm being obtuse – I think some of the comments and tests are documented as working the opposite way from how they actually work.

For example, the comment for silenceFilter is:

// silenceFilter is a function that returns true if a silence
// should be dropped from a result set for a given time.
type silenceFilter func(*pb.Silence, *Silences, time.Time) (bool, error)

The important phrase:

returns true if a silence should be dropped from a result set

And the tests for some of the filters support this:

func TestQMatches(t *testing.T) {
	...
	cases := []struct {
		sil  *pb.Silence
		drop bool
	}{
		{
			sil: &pb.Silence{
				Matchers: []*pb.Matcher{
					{Name: "job", Pattern: "test", Type: pb.Matcher_EQUAL},
				},
			},
			drop: true,
		},
	...
}

However, if I look in query where these functions are used:

func (s *Silences) query(q *query, now time.Time) ([]*pb.Silence, int, error) {
	...
		for _, f := range q.filters {
			ok, err := f(sil, s, now)
			if err != nil {
				return nil, s.version, err
			}
			if !ok {
				remove = true
				break
			}
	...
}

it seems that if the filter returns true, then it is instead included in the result set, and removed if it returns false. This is the opposite of what is in the comment:

returns true if a silence should be dropped from a result set

The tests for querying seem to support this too:

func TestSilencesQuery(t *testing.T) {
	...
		{
			// Retrieve all and filter
			q: &query{
				filters: []silenceFilter{
					func(sil *pb.Silence, _ *Silences, _ time.Time) (bool, error) {
						return sil.Id == "1" || sil.Id == "2", nil
					},
				},
			},
			exp: []*pb.Silence{
				{Id: "1"},
				{Id: "2"},
			},
		},
	}
...
}

because if "returns true if a silence should be dropped from a result set" were true, then the IDs 1 and 2 would be excluded from the result, rather than expected.

I just wanted to check that I'm not confused before I open a PR to fix this.