Proposal/Idea: Decouple upstream and downstream metrics

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

I have been thinking about a problem I’ve been trying to solve with respect to Redis upstream command stats, and I think there might be an opportunity for a more general solution to decouple stats for downstreams and upstreams.

The Redis-specific problem is that per-command stats were downstream specific and did not reflect traffic mirrored to multiple upstreams ie 1 downstream N upstreams. The mirroring use-case can have both M downstreams- health-checks, multiple proxies, discovery- and N upstreams. I implemented upstream-specific RedisCommandStats and left a todo to organize them better- ie perhaps use a singleton so as to avoid passing down the stats object through every layer of the Redis filter. However, given that we have multiple filters- proxies (multiple, especially with mirroring), healthchecks, etc., it's still not ideal.

My idea is to make use of some sort of context that can be shared between multiple filters. At present, each Redis filter has its own connection pool per cluster, though Henry is working on sharing the connection pool (by using the standard tcp connection pool) between filters so as to reduce connection volume. Each filter has access to the Envoy cluster objects- which can either be a standard Envoy cluster, or a special version designed to handle the Redis Cluster protocol. It would be bad to add Redis-specific stats to the general case, but we could add a general-purpose map to ClusterInfo. This map could be used for filter-specific purposes, in this case adding a Redis-specific shared stats object for use between all of the Redis filters. This would allow us to share symbolized stat names and avoid passing around stats objects in the filters- we could just call the clusterInfo and get the stats object.

Questions:

• Is stats upstream/downstream decoupling useful for stuff outside of Redis?
• Would adding the metrics to the cluster be useful for code simplification outside of Redis?

I’ve put up a draft PR that demonstrates the basics of registering upstream specific data on the more general cluster object. This allows you to register at cluster setup time various classes that you want to use in specific filters, and then at request time instead of doing a lazy load with lock just check if whatever upstream-specific class your filter uses is actually there, and if null ignore. I would greatly appreciate feedback on if this is a worthwhile idea and a reasonable approach: HenryYYang#7

[mattklein123]

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg from the description above it's not completely clear to me where you are trying to share data and what data you are trying to share. We already have quite a few mechanisms that can be used to share data within Envoy. Can you potentially more briefly describe what you want to share between filters, how long it should live, etc? cc @jmarantz also. Thank you.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Sorry for the delay!

Use Case:

The specific case I'm thinking of is sharing a RedisCommandStats (Redis-specific upstream stats) object between various filters.

Imagine we have two Redis listeners, A and B, and two Redis clusters, 1 and 2, where:

• A points to 1
• A mirrors 100% of traffic to 2
• B points to 2

A and B have their own upstream stats objects, and have to pass these down in code to the point where we measure things, and there are other stats objects too (really a cleanup task to migrate to the new style stats across the board for Redis).

The use case of mirroring is applicable not just to migrations, but to backup systems and cross-region replication. It could also help with prefix routing.

What is being shared, how long dooes it live, etc?

I am proposing that we add the upstream stats object onto the cluster, and then when filters need to record latencies or counters for the upstream, they can use that directly.

Besides the appeal of the upstream stats living on the upstream, this means the stats object will for sure exist when a downstream is trying to send data upstream. If a particular listener owns the stats object, other listeners won't know if has been created, so we'd have to have some other sharing mechanism. And if we have that, why not put upstream stats on the upstream proper?

[mattklein123]

cc @jmarantz to advise on the above. IMO we should try really hard to not generically add new things to the cluster as it's already such a big and memory intensive thing.

Could you have a Redis stats singleton manager object that handles this type of sharing?

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

Adding a general stats manger singleton for Redis was @jmarantz suggestion- after discussing a bit with Henry I figured I'd ask about the more general upstream case.

If it is that expensive then a dedicated Redis stats singleton is definitely better.

[mattklein123]

I think it's not completely clear to me what the general case here is. If we can clarify that happy to discuss further.

[FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg]

The general case is where downstreams in different filters point to the same upstream. However, if that's not really that common, it's not a big deal.