Introducing Metrics APIs

[tarekgh]

Fixes #44445.

dotnet/designs#211

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[tarekgh]

@ericstj @ViktorHofer could you please have a quick look at the changes in the versions.props and csproj?

[tarekgh]

CC @reyang @cijothomas @victlu

[tarekgh]

CC @shirhatti

[ViktorHofer]

@ericstj @ViktorHofer could you please have a quick look at the changes in the versions.props and csproj?

The changes LGTM. Thanks @tarekgh. Just a quick note, we were discussing dropping old frameworks in projects which include DiagnosticSource, so this extra work might not be necessary anymore in 1-2 months.

cc @ericstj for the added System.Linq dependency as you mentioned it offline.

[noahfalk]

Thanks @tarekgh, this looks nice! I did find some issues we should resolve but certainly nothing that would merit missing getting this code checked in for Preview5.

[noahfalk]

Have you benchmarked this? This code is on the hot path and I suspect this implementation is slower and uses more memory than the array iteration that the prototype code used. I know some of the code in the prototype wasn't thread-safe but I think this part was.

[tarekgh]

I didn't measure and compare it with the prototype. I am not expecting the extra memory here is issue at all because it is proportional to the number of listeners which is small. Also, the prototype was reallocating a new array with every operation on the subscription which would cause more temporary allocation than the current implementation. Perf wise I expect the current implementation would be faster than using the arrays. The reason is arrays need to check the bounds every time accessing any item while current implementation doesn't. I'll try to do some measurements after merging these changes and we can enhance more if needed or found any issue.

[jkotas]

I agree with @noahfalk. Arrays tend to have much better memory locality. The well-predicted bounds check is many times cheaper than a CPU cache miss.

the reason is arrays need to check the bounds every time accessing any item while current implementation doesn't

Was the original prototype code accessing the array in a loop where the bounds-check would be optimized out?

[tarekgh]

That is right. the original prototype was using the array in the loop https://github.com/noahfalk/opentelemetry-dotnet/blob/80e6ff71dc44cea192c4f0ceb14e63a024c7d155/example3/Microsoft.Diagnostics.Metric/MeterInstrument.cs#L231 so it could be optimized out. As I mentioned I didn't compare both yet, but this is something I'll do later outside this PR.

[tarekgh]

I agree with @noahfalk Noah Falk. Arrays tend to have much better memory locality. The well-predicted bounds check is many times cheaper than a CPU cache miss.

Note that the list would be small any way as I am expecting in most regular cases, we'll have one or two items at most in the list.

[TonyValenti]

@tarekgh - I noticed that histogram has methods named "Record". This word has two different meanings and pronunciations - "please secure this medical record" and "please record this Teams meeting".

I think the intended "record" is the second one. To avoid ambiguity, I would recommend renaming those methods to something like "Capture", "Collect", or "Track".

[tarekgh]

@TonyValenti we are following OpenTelemetry specification https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#record. I would suggest you discuss this naming with OpenTelemetry and we can adjust as needed.

CC @reyang

[TonyValenti]

Thanks! I just opened a discussion with them.

[stephentoub]

What does "dynamically" mean here?

[tarekgh]

I think this is originally introduced to address the ask in the comment https://github.com/dotnet/runtime/pull/40544/files#r467197582

[stephentoub]

Ok... what does it mean? :-)

[tarekgh]

My understanding is:

Originally we had

public Enumerator<KeyValuePair<string, object?>> GetEnumerator() => new Enumerator<KeyValuePair<string, object?>>(_first); 

which was not reachable by the public APIs and was causing the linker to trim it. The proposed fix was to add the attribute [DynamicDependency(DynamicallyAccessedMemberTypes.PublicMethods, typeof(TagsLinkedList))] to stop the linker from trimming it. @eerhardt suggested a better solution to change the enumerator implementation to something like:

        public Enumerator<T> GetEnumerator() => new Enumerator<T>(_first);
        IEnumerator<T> IEnumerable<T>.GetEnumerator() => GetEnumerator();
        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

So, the comment describing that. I guess the word dynamically is picked up from the attribute name DynamicDependency.

CC @CodeBlanch if he can clarify more.

[jkotas]

What are the consumers that are accesing this field dynamically (I assume that it means reflection)?

The burden is on the consumers to ensure that the method is not trimmed when they access it via reflection.

[eerhardt]

The burden is on the consumers

IIRC, the issue was the linker was removing the non-public, unused method during our build of the library. And so the method wasn’t even ending up in our shipped assembly.

I question whether this scenario is worthwhile, since calling the method through reflection is going to box the struct anyway as an ‘object’ return. But I remember someone had reasoning for doing this.

[CodeBlanch]

In OpenTelemetry we build a DynamicMethod that will access the struct enumerator and loop on it without boxing: https://github.com/open-telemetry/opentelemetry-dotnet/blob/a0fcbc0a35ad63ff21f1efcda45491570841985b/src/OpenTelemetry.Api/Internal/EnumerationHelper.cs#L79

Usage example: https://github.com/open-telemetry/opentelemetry-dotnet/blob/a0fcbc0a35ad63ff21f1efcda45491570841985b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs#L75

[jkotas]

It is a bad practice to access framework private implementation details using reflection. Our default stance is that it is not supported scenario. When we break library does access private implementation details using reflection, we consider it by design.

Could you please delete the BindingFlags.NonPublic from
https://github.com/open-telemetry/opentelemetry-dotnet/blob/a0fcbc0a35ad63ff21f1efcda45491570841985b/src/OpenTelemetry.Api/Internal/EnumerationHelper.cs#L167

If you open telemetry needs to access this enumerator method, we should make it official public API instead.