Revisit the use of generics in Metric instruments

[cijothomas]

Parent: #316
With generic metric instruments, the aggregators has to do multiple checks/casts for every metric update as in below example code. JIT may optimize this, but this needs to be discussed to see if it makes sense to get rid of generics.

T sum;
public override void Update(T value)
        {
            // TODO discuss if we should move away from generics to avoid
            // these conversions.
            if (typeof(T) == typeof(double))
            {
                this.sum = (T)(object)((double)(object)this.sum + (double)(object)value);
            }
            else
            {
                this.sum = (T)(object)((long)(object)this.sum + (long)(object)value);
            }
        }

The above would be re-written as

long sum;
public override void UpdateInt64(long value)
        {
                this.sum = this.sum + value;            
        }
...

This would obviously involve copy pasting code for Int64 and Double implementations, but it may be better alternative.

[cijothomas]

@tarekgh please share you thoughts on this when you are back from vacation.

[lmolkova]

I share the same concern for Counter and Gauge

        protected override Counter<T> CreateCounter<T>(string name, bool monotonic = true)
        {
            if (typeof(T) != typeof(long) || typeof(T) != typeof(double))
            {
                throw new InvalidOperationException();
            }

            // return no op
            throw new System.NotImplementedException();
        }

        protected override Gauge<T> CreateGauge<T>(string name, bool monotonic = true)
        {
            if (typeof(T) != typeof(long) || typeof(T) != typeof(double))
            {
                throw new InvalidOperationException();
            }
    }

this does not seem right and I think implementing long and double counters is a much better alternative as it makes users' life better.

[tarekgh]

As I mentioned before the JIT is really optimizing that so no perf hit with this pattern. Also, the framework uses this patterns in some places so this is not something new here. In addition to that, the compiler guys are already discussing how to simplify this and I am expecting in the near future compiler versions will allow easier pattern that don't need to do the type checks. At that time we can just update the implementation by then.

I don't like to expose a different class for every supported type. in addition the Meter has the methods that explicit returning the desired counter type. so users/developers are not really going to deal with the generic. If in the future we need to support more counter types (integer counters for instance) we'll not have to expose fully new class for that, instead we'll just expose a simple method in the Meter.

[cijothomas]

I remember you mentioned JIT optimizes this, and I understand that end users are less impacted as they call public override Counter<long> CreateInt64Counter(string name, bool monotonic = true).

However, I faced some issues with use of generics while implementing SDK. I could be implementing these wrong, but i'd like to get opinions on the below:

The MeterSDK should keep list of all active instruments, and hence I tried to do something like this in MeterSDK.cs.

private readonly IDictionary<string, CounterSDK<T>> counters = new ConcurrentDictionary<string, CounterSDK<T>>();

The above cannot be compiled. This can only work if MeterSDK is also generic like MeterSDK<T> which didn't make sense to me as MeterSDK can have both long and double instruments.

I worked around this by doing separate dictionary for long and double as shown below in MeterSDK.cs

private readonly IDictionary<string, CounterSDK<long>> longCounters = new ConcurrentDictionary<string, CounterSDK<long>>();
private readonly IDictionary<string, CounterSDK<double>> doubleCounters = new ConcurrentDictionary<string, CounterSDK<double>>();

This essentially means, we'll have to modify lot of code if we decide to add a new type(int) in the future.
@tarekgh Could you comment on the above approach? If the above approach is my only option, then we are not gaining much by using generics . Otherwise, I'd love to learn alternate approaches.

[tarekgh]

Could you comment on the above approach?

Your approach looks reasonable but would be nice to tell more details to think if there is a better approach. like, when you'll will cache the counter object? and the dictionary key string is unique for this type of counter or it can be used as a key of different types of counters.

If the above approach is my only option, then we are not gaining much by using generics .

I disagree with that. You are mixing the concept of the public APIs and the implementation details. What you are talking about here is the implementation details. I am concerned more about the public APIs. If we didn't go with the generic approach, we'll have to double the number of the counter classes we expose in addition most of the implementation of these classes will be just a duplication as the functionality is exactly the same and the difference is only in type of the class. why you think the duplication is better?
Also, if we decided to expose Counter of integer type in the future, we'll have to expose a fully new class which usually nobody will use except Meter will instantiate it. In addition you'll need to reduplicate the implementation too.
So, Generic is really beneficiary here. In addition, you are not gaining anything more when not using Generics.

[cijothomas]

If MeterSDK's current approach of keeping separate Dictionary instances for longCounters and doubleCounters is the right way, then when we introduce (if we ever) intCounter, MeterSDK will need to be changed to have this new Dictionary instance. So the implementation still needs to change a lot of places to accommodate new type. (I agree user API won't require change) I am seeing that using generic has not saved any lines of code in the implementation.

Implementation is harder whenever I encounter generics. Apart from the examples shared above, how would one use Interlocked classes with generic?
CompareExchange works only when T is reference type, but in our case we have T restricted to value type only. This will force the usage of other overloads of CompareExchange - one for long, one for double, one for int (if we have int in future). This basically means - the implementation is different for each T - (Not same code as you were hinting). This lead me to the statement that using Generics is not saving me any lines of code. Instead its making the implementation harder.

Due to the usage of Generics in the API, the SDK is forced to have generics and this is affecting implementation.

[tarekgh]

I am seeing that using generic has not saved any lines of code in the implementation.

It does save as we'll not introduce a new class and implement it. Also the implementation changes would be very minimal than handling fully a new class.

Implementation is harder whenever I encounter generics. Apart from the examples shared above, how would one use Interlocked classes with generic?

Where you are going to call the interlock operation? and why? if you are calling it inside the counters, then we already have the type check there which make it easy to know which overload you want to use for the interlock. you are going to do that anyway even if we don't have generics. I mean you'll have to use different interlock overload regardless if having generics or not.

Due to the usage of Generics in the API, the SDK is forced to have generics and this is affecting implementation.

Yes but what is wrong with that?

I see your point the generics adding some little overhead in the implementation but I am not really seeing it is a big deal.

I am not going to push back more on that, if all agree preferring not having generics, then change the classes to non-generic (by adding at least 6 more classes) but I don't prefer that just because of the implementation details which we are really controlling and not expecting to be complicated either.

[cijothomas]

Interlocked will be used in aggregators. (see a sample aggregator https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Metrics/Aggregators/CounterSumAggregator.cs)
I am changing the Aggregator from its current (incorrect) implementation to do Sum in thread safe way.

With generic, I cannot find a way to implement the same. Without generic, Interlocked.Add(this.sum,value) can be used (for long). How would I achieve this with generic T ?

For Double, I intend to use the Interlocked.Exchange (https://docs.microsoft.com/en-us/dotnet/api/system.threading.interlocked.compareexchange?view=netframework-4.8#System_Threading_Interlocked_CompareExchange_System_Double__System_Double_System_Double_) to accumulate sum. I am not sure how to get this with generic T?

[cijothomas]

@tarekgh The following is how I write Aggregator without generic. (simplified version) I am yet to find how to implement the same with generic involved here.

public class LongCounterSumAggregator
    {
        private long sum;
     
        public void Update(long value)
        {
            Interlocked.Add(ref this.sum, value);
        }
     }

And for double:

public class DoubleCounterSumAggregator
    {
        private double sum;        
        public void Update(double value)
        {
            double initialTotal, computedTotal;
            do
            {                
                initialTotal = this.sum;                
                computedTotal = initialTotal + value;
            }
            while (initialTotal != Interlocked.CompareExchange(ref this.sum,
                computedTotal, initialTotal));
        }
    }

Agree this is implementation and no API, but the above looks simple to read and reason about. I am comparing it with the below code I have currently with generic. This code needs to be modified to use Interlocked as well, which I am yet to figure out how.

            if (typeof(T) == typeof(double))
            {
                this.sum = (T)(object)((double)(object)this.sum + (double)(object)value);
            }
            else
            {
                this.sum = (T)(object)((long)(object)this.sum + (long)(object)value);
            }

As you can seen, there will be as many code blocks as the number of types for T we allow. (2 currently). So there is no "same code only difference is Type" - which led me to believe generics are not helping here.

[tarekgh]

@cijothomas feel free to use non-generic. I have some ideas can achieve implementing it as generic but it has to have the type checking blocks which you don't like. are we talking here about the aggregator classes or you want to convert all counter classes too? I am just checking to know the scope of the change.

[tarekgh]

By the way, I may not be able to respond quickly so if you get blocked, don't wait and do the change you think unblock you. thanks for your help.

[cijothomas]

For now my goal is to get aggregators implemented correctly - I will move to use non-generics for aggregators for now to unblock and make progress. There may be other areas which might face similar issues.

Lets keep this issue open. When you get a chance, please respond on how to achieve the aggregator implementation (shared above) with use of generics.

[tarekgh]

Here is one of the ideas how you can implement the generic version:
For full framework compilation, you may need to add reference to the NuGet package https://www.nuget.org/packages/System.Runtime.CompilerServices.Unsafe/

    public class CounterSumAggregator<T>
            where T : struct
    {
        private T sum;
        private T checkPoint;

        public void Checkpoint()
        {
            this.checkPoint = this.sum;
        }

        public void Update(T value)
        {
            if (typeof(T) == typeof(double))
            {
                double initialTotal, computedTotal;
                do
                {
                    initialTotal = (double)(object)this.sum;
                    computedTotal = initialTotal + (double)(object)value;
                }
                while (initialTotal != Interlocked.CompareExchange(ref Unsafe.As<T, double>(ref this.sum), computedTotal, initialTotal));
            }
            else
            {
                Interlocked.Add(ref Unsafe.As<T, Int64>(ref this.sum), (long)(object)value);
            }
        }

        public T ValueFromLastCheckpoint()
        {
            return this.checkPoint;
        }
    }

Note, I didn't look at the generated code to see if there will be any perf issue with this approach but I can do that later when I am back.

[cijothomas]

Thanks! Let me try the above.

[tarekgh]

@cijothomas is there anything blocking you to complete this work? do we still need to discuss using Generics?

[MikeGoldsmith]

I believe @cijothomas is on PTO, not sure when he's due back. I agreed with @SergeyKanzhelev to pick this up and review what's outstanding once I've finished my OTEP-66 prototype (if Cijo isn't available). I hope to get to this by the end of the week.

[tarekgh]

Thanks @MikeGoldsmith for the info. I'll wait then and let me know if there is any issue I can help with.

[cijothomas]

@tarekgh I am back from parental and continuing metrics work.
This issue needs resolution before we ship beta.

You mentioned you haven't checked performance when using Unsafe.As conversions. We definitely want to know if there is a perf hit due to this as this conversions occur for every metric update and is a hot path - Can you help check this?

I'd prefer to move away from Generics and implement separate LongCounter, DoubleCounter etc. My reasoning are:

1. Code is neat and clean to read, without any conversions.
   eg:
   Interlocked.Add(ref this.sum, value);
   vs
   Interlocked.Add(ref Unsafe.As<T, long>(ref this.sum), (long)(object)value);

2. Even with generics, we still needed to write separate code for long, and double. This, in my opinion, is largely negating the benefits of using genrics.
   eg:

            if (typeof(T) == typeof(double))
            {
                this.checkPoint = (T)(object)Interlocked.Exchange(ref Unsafe.As<T, double>(ref this.sum), 0.0);
            }
            else
            {
                this.checkPoint = (T)(object)Interlocked.Exchange(ref Unsafe.As<T, long>(ref this.sum), 0);
            }

3. Usage of generics require us to take a new dependency on System.Runtime.CompilerServices.Unsafe.

4. Possible performance issues arising from the conversions back and forth. I haven't quantified, but this may be a non-issue if Tarek confirms no perf hits.

5. minor - other platforms like Java is doing LongCounter, DoubleCounter instead of generics.

[tarekgh]

@cijothomas to be honest none of the listed reasons really block from using Generic. As I mentioned before Generic will give the benefit not exposing more types. you'll need to expose at least 6 more types with the none generic and in the future, you'll need more if you want to support other types. In addition, as mentioned before, developers are not going to use the Generic directly. I was trying to look at what really is blocking.

Here are some more notes just to clarify:

• Unsafe.As should be very optimized anyway and I am not really expecting any real perf issue with it. but if you still want me to get you some numbers I can try to do that.
• taking a dependency on System.Runtime.CompilerServices.Unsafe is not an issue especially since this library has a lot more things that could be needed for other scenarios. in addition there are some BCL libraries already using it which not really going to add anything to the cost.
• Java generics are not really friendly with types like long, double. you have to use the wrapper types there. This can be part of the decision there.
• I am not seeing use Unsafe code is adding much complexity. The code is explicit to understand we are doing the casting and readable.
• if we are writing code anyway when not using generic so why it is a problem to do so when using a generic? Note that in the future if we get the compiler changes for generic with value types that could minimize the code writing.

Now, I am not going to push back if you want to go ahead and use non-generic versions. feel free to do so if you believe this better.

[MikeGoldsmith]

I am in favour replacing generic types with concrete types - although they could wrap the generic type. Generics used in this way are not easy to understand and don't form a user-friendly interface.

I feel it also complicates metric processing with multiple typoeof() checks to determine code path.

I think a mixed approach of concrete and generics could work well. Something along the lines of:

internal abstract class MetricBase<T> {}
public class LongMetric : Metric<long> {}
public class DoubleMetric : Metric<double> {}

This would allow us to utilise generics and avoid many of the type checks and hopefully simplify code.

[tarekgh]

@cijothomas @SergeyKanzhelev feel free to get rid of the generic. I don't think mixing approach will buy us much if we just use non-generic all the time. we'll have to expose other classes anyway. Feel free to close this issue when you do so and let me know if you have any questions.

I thought when exposing the nongeneric instances from the Meter was enough but looks this is not the case.

[zahirtezcan]

Not sure if I am allowed to comment but, you may also consider using a union type for sum via LayoutKind.Explicit and setting FieldOffsets to zero.

If that sounds stupid just ignore me. I am crossing my fingers for years waiting for extension interfaces though. See section An example: Numeric abstraction.