[common][router][WIP] cache dimensions for otel

[m-nagarajan]

Summary

Problem:
Currently, every metric.record() call creates a new Attributes object with all the dimensions needed for that metric. As it also happens on the happy path, it will lead to high rate of object churn and potentially affecting GC.

Solution:
This PR aims at reducing this object churn.
Considered 2 approaches and end up choosing approach 2 to cache the dimensions:

1. Pre create all possible dimensions: This gets complicated as store name and cluster name are also part of the dimensions and it can lead to creating so many Attributes object and we need to craft a key during runtime to get to the precreated dimensions. Precreating everything is not possible as there can be new stores coming into the picture after bootstrap.
2. Cache the dimensions: Rather than pre creating all the dimensions we can create them as and when needed and then cache it for future uses. This will be more dynamic without the need for precreating all combinations. Similar key is needed to access the cache.

Implementation details:

1. using a ThreadLocal<Map<VeniceMetricsDimensions, String>> to pass in the dimension and its values rather than building an object everytime or pass using varargs or writing custom methods for each metrics
2. using a VeniceConcurrentHashMap<String, Attributes> to cache the unique Attributes
3. key (of type String)to access this cache is the combination of all dimension names and values. Eg: "DIMENSION1NAMEdimension1valueDIMENSION2NAMEdimension2value..."
4. Modified dimensionsList in MetricEntity from Set to a SortedSet to help in creating consistent keys.
5. Every RouterHttpRequestStats (or potentially any stats object class) will create its own VeniceOpenTelemetryDimensionsCache to take advantage of the base dimensions.
6. For the key, I originally was pre-creating a pattern for each MetricEntity like "DIMENSION1NAME%sDIMENSION2NAME%s..." but that needs using String.format(), so ended up using a StringBuilder and creating the full string during runtime instead.

How was this PR tested?

NA

Does this PR introduce any user-facing changes?

• No. You can skip the rest of this section.
• Yes. Make sure to explain your proposed changes and call out the behavior change.

[lluwm]

Why we can assert dimensionValue is always in reusableDimensionsMap? Is it because we always filling in the content before calling checkCacheAndGetDimensions or something else? Would it be intuitive to move the codes inside this function, I mean the codes that inserts dimension values into the cache. If we could do that, then probably we don't need to expose the getThreadLocalReusableDimensionsMap to public.

If TODO comments is still valid and we never clear the cache after use, is there any reasonable limit value that we can cap the size of this cache?

[lluwm]

Is it correct that, ThreadLocal requires every router thread servicing a client query have to create a copy of this map? If it is the case, do we know the upper limit of how large the thread number could be?

[FelixGV]

Left a high level comment... Please take a look and LMK what you think.

[FelixGV]

This logic looks extremely complex to me, both in terms of my ability to understand it, but also in terms of time complexity (so many map operations, string building, etc...).

Since the MetricEntityState retryCountMetric is a private property of this class, and this class has the per-store scope that we're interested in, I don't understand, why not cache the Attributes dimensions inside of the MetricEntityState object itself? Then this whole function can become simply retryCountMetric.record(1); and we let the dimension-passing be completely handled on the inside, by simply taking it from some private final Attributes dimensions property of the MetricEntityState... No map lookups, no string building, no threadlocal state... all of that disappears completely. And by hiding the OTel complexity in this way, we should greatly simplify the migration from Tehuti to OTel.

WDYT?

[m-nagarajan]

Thanks @FelixGV for the comment. Packing the cache inside MetricEntityState by relying on the existing class with the per-store scope sounds good as well, but it doesn't eliminate the string building (for cache key) completely as each state can have multiple Attributes keyed by one/more of the varying dimensions. For instance,

1. retryCountMetric can have multiple Attributes based on values of RequestRetryType
2. healthyRequestMetric can have multiple Attributes based on values of HttpResponseStatus and HttpResponseStatusCodeCategory.
   Also, when we move away from tehuti we will regroup some of the current MetricEntityStates into 1 (for instance: healthy_request, unhealthy_request, tardy_request, etc to just 1) which is going to increase this cardinality further. We can continue keeping each of these combinations to be a separate MetricEntityState, but I feel like its too much denormalizing and we have to further denormalize it to keep things 1:1. In other alternative routes, we will need some form of key to access the cache (global or per RouterHttpRequestStats or per MetricEntityState ). The content of the key gets smaller as we move further away from global cache, the easier being just 1 dimension like in retryCountMetric where a cache per RouterHttpRequestStats like below would work.

VeniceConcurrentHashMap<RequestRetryType, Attributes> otelDimensionCacheForRequestRetryType;

If not, if we have combinations of two or more dimensions as keys, we need to construct some form of key for the cache. What do you think?

[FelixGV]

I don't see a problem with the denormalization... We can make each MetricEntityState instance carry a single Attributes property, but have multiple MetricEntityState hold a reference to the same otelMetric. With this design, the complete attribute specification is done upfront, in each MetricEntityState, and we have (IMHO) a clear decoupling of the underlying metric stack. IOW, different MetricEntityState instances can hold references to a single otelMetric while each also holds a reference to a different tehutiSensor (since in Tehuti the dimensions are expressed as part of the sensor's name).

The best cache is no cache at all, just an object pointer. That is the fastest. I may be missing context about cases where this will not be good enough and where we will still need a cache, but even if such cases do exist, we still ought to make the simple cases as fast as possible.

[FelixGV]

More generally... I think we need to be careful about low-level optimizations such as this one. The goal of this PR is to eliminate the instantiation of a new Attributes object on each metric recording. That is a good goal in and of itself, but there are two ways to approach it:

1. We optimize it in a way that is clearly beneficial. For example, replacing an instantiation by a pointer lookup is clearly faster. No doubt about it. But replacing the Attributes instantiation by all of the stuff going on in this PR, including string building, map lookups, etc. is not clearly better. It may be better, or neutral, or perhaps even worse, and that leads me to the next point...
2. We need to do some form of benchmarking, either via micro-benchmarking or running the E2E system with a profiler or some other method in between. That is what we need to do when it's ambiguous whether the optimization is better than the incumbent implementation.

[gaojieliu]

I like the data-driven approach.
There is a trade-off regarding performance vs maintainability.
The complex logic is not always good unless it can prove much better performance as complex logic incurs more maintenance overhead in the future and more error prone.

[FelixGV]

So... thinking about it some more, I'd like to propose the following:

1. Let's make OTel dimensions fully encapsulated inside of MetricEntityState and its subclasses (see next point). No other code path should ever instantiate an OTel Attributes. This will help make the migration easier.
2. Let's make MES abstract, with an API intended for use (but NOT for extension) by subclasses: protected final void record(value, attributes).
3. Let's provide a few subclass options for common cases. For example, one subclass will have just one immutable set of dimensions, stored as a private final Attributes class prop, and expose a public void record(value) function which internally passes the class prop attributes into the parent's protected function. Another subclass could take an enum as constructor param and instantiate an EnumMap with Attributes value (an EnumMap lookup is cheaper than a Map lookup, and it's inherently threadsafe in the sense that the backing array will never resize); a public void record(value, enum) function would perform the EnumMap lookup and pass the attributes into the parent's protected function. We can also do other subclasses as needed (e.g. based on 2 enums, 3 enums...).
4. No thread local state.
5. No map lookups (besides EnumMap).
6. No instantiation (of strings or anything else).

IMHO, such an approach will be simple to reason about and faster than both the current impl and the one in this PR.