rethink request ID generation wrt runtime routing

[mattklein123]

As of #188, we use a process wide incrementing ID for the request ID (which is used as the stable random number during runtime routing).

This is good for performance reasons (and in many deployments provides effectively random behavior), but the behavior is confusing for newcomers. We should make the new and old behavior configurable.

[mattklein123]

cc @rshriram I thought about this and I think how I plan on handling is to detect whether the route table has any runtime rules in it. If it does, I will use the old behavior, if not, I will use the new behavior. I think this is a good compromise. Will fix this week.

[rshriram]

@mattklein123 I was about to respond to this as well. How about always letting the user specify the weights in the intuitive way (v2=30, v3=20, unweighted block for v1). Internally, in the code, we could create the desired representation in monotonically increasing weights (v2_ordered=30, v3_ordered=50, v1_ordered=100). The current logic that you have for generating requestIDs, with the modulo 100 can remain the same. This is what I currently do in the nginx sidecar in amalgam8.

Additionally, do you happen to have some numbers on how much perf this globally shared ID provides over a thread-local pseudo random number generator? The problem with the shared ID is cache line contention, especially when you scale to multiple cores across sockets. With a thread-local pseudo random number generator, it will be additional instructions to process but if its all bitops, it should be relatively cheap.

[mattklein123]

Changing how the weights are specified is a larger change and we can discuss that as a separate item. In general it is unlikely we will change the current behavior of having the routes be related. If we do anything, we will make a new routing rule type that allows the specification of multiple cluster targets with associated weights.

[rshriram]

To clarify, I was not advocating for a new way of specifying rules. What I actually meant was something like this:

"routes": [
                    {
                      "runtime" : {
                          "key" : "service1.v2",
                          "default" : 30
                      },
                      "prefix": "/service/1",
                      "cluster": "service1-v2"
                    },
                    {
                      "runtime" : {
                          "key" : "service1.v3",
                          "default" : 30
                      },
                      "prefix": "/service/1",
                      "cluster": "service1-v3"
                    },
                    {
                      "timeout_ms": 0,
                      "prefix": "/service/1",
                      "cluster": "service1"
                    }
   ]

The rest of the suggestion i made earlier stays the same. There is no syntax change for the user. Its just how the weights are computed internally across different route blocks.

[mattklein123]

I understand, but that then links all of the routes together and they then need to be pre-processed. This also changes existing behavior that is being depended on (each route is unique). We will keep the existing behavior and fix the random generation. We can then separately discuss whether we want to have easier constructs for this specific case (effectively sending the same route to multiple clusters based on %). That might look like:

{
...
"weighted_clusters:" [
{ "name": "cluster1", "weight": 33 },
{ "name": "cluster2", "weight": 33 },
{ "name": "cluster3", "weight": 33 },
]
}

[mattklein123]

fix for main issue is here: #246

I will split out the feature request (multi-cluster weighted routing) into a new issue.

[mattklein123]

fixed