Add (optional) names to routes

This commit adds (optional) names to routes to fix prometheus#3817.
In the case where a user has two routes with the same
receiver, matchers and group_by, a name can be used to
ensure their groups have unique group keys and avoid using
the same nflog.

Signed-off-by: George Robinson <george.robinson@grafana.com>

config/config.go

@@ -549,6 +549,9 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	if c.Route == nil {
 		return fmt.Errorf("no routes provided")
 	}
+	if len(c.Route.Name) > 0 {
+		return errors.New("root route cannot have a name")
+	}
 	if len(c.Route.Receiver) == 0 {
 		return fmt.Errorf("root route must specify a default receiver")
 	}
@@ -558,11 +561,15 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	if len(c.Route.MuteTimeIntervals) > 0 {
 		return fmt.Errorf("root route must not have any mute time intervals")
 	}
-
 	if len(c.Route.ActiveTimeIntervals) > 0 {
 		return fmt.Errorf("root route must not have any active time intervals")
 	}
 
+	// Validate that the route name is unique at the same level.
+	if err := checkRouteNames(c.Route); err != nil {
+		return err
+	}
+
 	// Validate that all receivers used in the routing tree are defined.
 	if err := checkReceiver(c.Route, names); err != nil {
 		return err
@@ -588,6 +595,31 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return checkTimeInterval(c.Route, tiNames)
 }
 
+// checkRouteNames checks that the names of the route are unique
+// at each level in the routing tree.
+func checkRouteNames(r *Route) error {
+	if len(r.Routes) == 0 {
+		return nil
+	}
+	seen := make(map[string]struct{})
+	for _, cr := range r.Routes {
+		// Name is optional.
+		if cr.Name == "" {
+			continue
+		}
+		if _, ok := seen[cr.Name]; ok {
+			return fmt.Errorf("route name %q is not unique", cr.Name)
+		}
+		seen[cr.Name] = struct{}{}
+	}
+	for _, cr := range r.Routes {
+		if err := checkRouteNames(cr); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // checkReceiver returns an error if a node in the routing tree
 // references a receiver not in the given map.
 func checkReceiver(r *Route, receivers map[string]struct{}) error {
@@ -776,6 +808,7 @@ func (c *GlobalConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 
 // A Route is a node that contains definitions of how to handle alerts.
 type Route struct {
+	Name     string `yaml:"name" json:"name"`
 	Receiver string `yaml:"receiver,omitempty" json:"receiver,omitempty"`
 
 	GroupByStr []string          `yaml:"group_by,omitempty" json:"group_by,omitempty"`

config/config_test.go

@@ -147,6 +147,63 @@ receivers:
 	}
 }
 
+func TestRouteNameIsUnique(t *testing.T) {
+	testCases := []struct {
+		name        string
+		in          string
+		expectedErr string
+	}{{
+		name: "duplicate names are rejected",
+		in: `
+receivers:
+  - name: test
+route:
+  receiver: test
+  routes:
+    - name: foo
+    - name: foo
+`,
+		expectedErr: "route name \"foo\" is not unique",
+	}, {
+		name: "unique names are permitted",
+		in: `
+receivers:
+  - name: test
+route:
+  receiver: test
+  routes:
+    - name: foo
+    - name: bar
+`,
+	}, {
+		name: "duplicate names under separate parents are permitted",
+		in: `
+receivers:
+  - name: test
+route:
+  receiver: test
+  routes:
+    - name: foo
+      routes:
+        - name: baz
+    - name: bar
+      routes:
+        - name: baz
+`,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := Load(tc.in)
+			if tc.expectedErr == "" && err != nil {
+				t.Fatalf("err returned, expected nil:\n%q", err)
+			} else if tc.expectedErr != "" && tc.expectedErr != err.Error() {
+				t.Errorf("\nexpected:\n%q\ngot:\n%q", tc.expectedErr, err.Error())
+			}
+		})
+	}
+}
+
 func TestMuteTimeExists(t *testing.T) {
 	in := `
 route:
@@ -445,6 +502,21 @@ receivers:
 	}
 }
 
+func TestRootRouteHasNoName(t *testing.T) {
+	in := `
+route:
+  name: test
+`
+	_, err := Load(in)
+	expectedErr := "root route cannot have a name"
+	if err == nil {
+		t.Fatalf("no error returned, expected:\n%q", expectedErr)
+	}
+	if err.Error() != expectedErr {
+		t.Errorf("\nexpected:\n%q\ngot:\n%q", expectedErr, err.Error())
+	}
+}
+
 func TestContinueErrorInRouteRoot(t *testing.T) {
 	in := `
 route:

dispatch/route.go

@@ -42,6 +42,9 @@ var DefaultRouteOpts = RouteOpts{
 type Route struct {
 	parent *Route
 
+	// The name of the route.
+	Name string
+
 	// The configuration parameters for matches of this route.
 	RouteOpts RouteOpts
 
@@ -123,6 +126,7 @@ func NewRoute(cr *config.Route, parent *Route) *Route {
 
 	route := &Route{
 		parent:    parent,
+		Name:      cr.Name,
 		RouteOpts: opts,
 		Matchers:  matchers,
 		Continue:  cr.Continue,
@@ -172,12 +176,15 @@ func (r *Route) Match(lset model.LabelSet) []*Route {
 // Key returns a key for the route. It does not uniquely identify the route in general.
 func (r *Route) Key() string {
 	b := strings.Builder{}
-
 	if r.parent != nil {
 		b.WriteString(r.parent.Key())
 		b.WriteRune('/')
 	}
-	b.WriteString(r.Matchers.String())
+	if r.Name != "" {
+		b.WriteString(r.Name)
+	} else {
+		b.WriteString(r.Matchers.String())
+	}
 	return b.String()
 }
 
@@ -190,7 +197,11 @@ func (r *Route) ID() string {
 		b.WriteRune('/')
 	}
 
-	b.WriteString(r.Matchers.String())
+	if r.Name != "" {
+		b.WriteString(r.Name)
+	} else {
+		b.WriteString(r.Matchers.String())
+	}
 
 	if r.parent != nil {
 		for i := range r.parent.Routes {

dispatch/route_test.go

@@ -854,7 +854,7 @@ routes:
 	}
 }
 
-func TestRouteID(t *testing.T) {
+func TestRouteKeyAndID(t *testing.T) {
 	in := `
 receiver: default
 routes:
@@ -894,12 +894,39 @@ routes:
 - continue: true
   matchers:
   - corge!~"[0-9]+"
+  name: corge
+  routes:
+  - matchers:
+    - corge=waldo
+    name: waldo
 `
 	cr := config.Route{}
 	require.NoError(t, yaml.Unmarshal([]byte(in), &cr))
 	r := NewRoute(&cr, nil)
 
-	expected := []string{
+	expectedKeys := []string{
+		"{}",
+		"{}/{foo=\"bar\"}",
+		"{}/{foo=\"bar\"}/{bar=\"baz\"}",
+		"{}/{foo=\"bar\"}",
+		"{}/{foo=\"bar\"}/{bar=\"baz\"}",
+		"{}/{foo=\"bar\"}",
+		"{}/{foo=\"bar\"}/{bar=\"baz\"}",
+		"{}/{bar=\"baz\"}",
+		"{}/{bar=\"baz\"}/{baz=\"qux\"}",
+		"{}/{bar=\"baz\"}/{qux=\"corge\"}",
+		"{}/{qux=~\"[a-zA-Z0-9]+\"}",
+		"{}/corge",
+		"{}/corge/waldo",
+	}
+
+	var actual []string
+	r.Walk(func(r *Route) {
+		actual = append(actual, r.Key())
+	})
+	require.ElementsMatch(t, actual, expectedKeys)
+
+	expectedIDs := []string{
 		"{}",
 		"{}/{foo=\"bar\"}/0",
 		"{}/{foo=\"bar\"}/0/{bar=\"baz\"}/0",
@@ -911,12 +938,13 @@ routes:
 		"{}/{bar=\"baz\"}/3/{baz=\"qux\"}/0",
 		"{}/{bar=\"baz\"}/3/{qux=\"corge\"}/1",
 		"{}/{qux=~\"[a-zA-Z0-9]+\"}/4",
-		"{}/{corge!~\"[0-9]+\"}/5",
+		"{}/corge/5",
+		"{}/corge/5/waldo/0",
 	}
 
-	var actual []string
+	actual = actual[:0]
 	r.Walk(func(r *Route) {
 		actual = append(actual, r.ID())
 	})
-	require.ElementsMatch(t, actual, expected)
+	require.ElementsMatch(t, actual, expectedIDs)
 }