Add the route ID to uuid

[qinxx108]

Adding a UUID to identify the route in general
The route can be UUID by itself matchers+parents matchers + itself position

                                                                         route1(matcher="1")
         
                    route2(matcher="1")                   route3(matcher="1")           route4(matcher="1")

route5(matcher="1")      route6(matcher="1")            

For example to UUID route 6, it will be (matcher="1")/(matcher="1")/(matcher="1")/1

[rohang98]

Should we include a line about how it's different than Key(), so something like, ID returns a unique key for the route. It is different than Key() as it adds the position in the list of parent routes.?

[simonpasquier]

I'm missing the context about this change. Is it linked to the pagination API?

[simonpasquier]

if we're only testing the top-level routes, there's no need for having sub-routes?
also it might be good to have routes with same key but different ids?

[qinxx108]

"I'm missing the context about this change. Is it linked to the pagination API?"

-> It is related as we need to UUID an aggregation group, aggregation group can be uuid by group_labels + routeId

[qinxx108]

=> if we're only testing the top-level routes, there's no need for having sub-routes?
also it might be good to have routes with same key but different ids?

Added the test

[simonpasquier]

(nit) not sure if we need to go into the implementation details here.

Suggested change

	// ID returns a key for the route. It should uniquely identify the route in general,
	// it is different than Key() as it adds the route's position on its parent's children.
	// ID returns a unique identifier for the route, unlike Key().

[qinxx108]

fixed

[gotjosh]

Overall direction looks good, but I have a couple of pieces of feedback:

1. I think we should directly make the ID an attribute of the Route struct. Routes structs are only created once once the Alertmanager starts or its configuration its updated and computing the IDs every single time is not needed as they don't change.

2. I think using the string representation of matches + position is a bit less human-friendly and provide poor UX. In particular, because we expect these to be used in URL that are meant to be be copy / pasteable by users. I suggest we do the following (and use a fnv hash on its hex representation)

// ID returns a unique identifier for the route, unlike Key().
func (r *Route) ID() string {
	hash := fnv.New64()

	var position *int
	if r.parent != nil {
		// Find the position in the same level leaf.
		for i, cr := range r.parent.Routes {
			if cr == r {
				position = &i
				break
			}
		}
	}

	_, _ = hash.Write([]byte(r.Key()))

	if position != nil {
		_, _ = hash.Write([]byte("/"))
		_, _ = hash.Write([]byte(fmt.Sprint(*position)))
	}

	return fmt.Sprintf("%016x", hash.Sum64())
}

3. I have a few comments on the readability of the test, but let's tackle the other two first, and then we can move into the test (which it's 👍 at an assertion level) later as those are mostly nits.

[qinxx108]

Overall direction looks good, but I have a couple of pieces of feedback:

1. I think we should directly make the ID an attribute of the Route struct. Routes structs are only created once once the Alertmanager starts or its configuration its updated and computing the IDs every single time is not needed as they don't change.
2. I think using the string representation of matches + position is a bit less human-friendly and provide poor UX. In particular, because we expect these to be used in URL that are meant to be be copy / pasteable by users. I suggest we do the following (and use a fnv hash on its hex representation)

// ID returns a unique identifier for the route, unlike Key().
func (r *Route) ID() string {
	hash := fnv.New64()

	var position *int
	if r.parent != nil {
		// Find the position in the same level leaf.
		for i, cr := range r.parent.Routes {
			if cr == r {
				position = &i
				break
			}
		}
	}

	_, _ = hash.Write([]byte(r.Key()))

	if position != nil {
		_, _ = hash.Write([]byte("/"))
		_, _ = hash.Write([]byte(fmt.Sprint(*position)))
	}

	return fmt.Sprintf("%016x", hash.Sum64())
}

3. I have a few comments on the readability of the test, but let's tackle the other two first, and then we can move into the test (which it's 👍 at an assertion level) later as those are mostly nits.

1. I think we should directly make the ID an attribute of the Route struct.
   => When a route gets created, we don't know its position as the other route in the same level in the tree hasn't finished creation yet. Add some comments about the ID function

2. use a fnv hash on its hex representation
   I will convert this to use hash before using it in the api, i was thinking about more readable for coding perspective before returning to customer: https://github.com/prometheus/alertmanager/pull/3364/files#diff-2ec4e2e390946f66324eb00bb9e735280a4d018e65de41e64bebac8bebebd23b. But i am find using hash in here as well, please let me know your thought

[gotjosh]

@qinxx108 if I remember correctly, this is the first PR that we're suppose to merge as part of the API - right? If so, could you rebase to fix the diff?

Thanks!

[qinxx108]

@gotjosh Yes this is the change
Rebased !

Thank you!

[gotjosh]

LGTM

Thank you very much for your contribution! I have addressed my own feedback.

[qinxx108]

@gotjosh wonder if we can merge this pr?

[gotjosh]

Yep, we can. Thanks again @qinxx108! Can you please rebase to fix added commits?

[grobinson-grafana]

Hi! 👋 I found a bug where route.ID() can return conflicting IDs. I've opened an issue here #3802.