Expose new EventCounter APIs #23808

sywhang · 2019-04-08T12:24:13Z

This exposes the new EventCounter APIs that were introduced in #23077, along with few changes that were discussed during API review in https://github.com/dotnet/corefx/issues/36129.

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/DiagnosticCounter.cs

Wraith2 · 2019-04-08T14:43:36Z

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/DiagnosticCounter.cs


        // arbitrarily we use name as the lock object.  
-        internal object MyLock { get { return _name; } }
+        internal object MyLock { get { return Name; } }


Is there a need for the second property to access name or could the lock just be on Name? MyLock also feels like a slightly odd name, in collections it would probalby be called SyncLock

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/CounterPayload.cs

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/DiagnosticCounter.cs

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/EventCounter.cs

jorive · 2019-04-09T17:18:21Z

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/EventCounter.cs

+                CounterPayload payload = new CounterPayload();
                payload.Count = _count;
                payload.IntervalSec = intervalSec;
                if (0 < _count)


I think this condition is correct for computing the Mean, but it should be _count > 1 for standard deviation.

jorive · 2019-04-09T17:25:44Z

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/EventCounter.cs

                {
                    payload.Mean = _sum / _count;
-                    payload.StandardDeviation = (float)Math.Sqrt(_sumSquared / _count - _sum * _sum / _count / _count);
+                    payload.StandardDeviation = Math.Sqrt(_sumSquared / _count - _sum * _sum / _count / _count);


Should we add a comment here about potential overflow for large values?

~~I'm not familiar with this formula, could you please add a reference to it?~~
[Edit] You are using the formula for a finite population with equal probabilities for all points :)

Optionally, it could be:

Math.Sqrt((1.0 / (_count * (_count - 1))) * (_count * _sumSquared - (_sum * _sum)));

noahfalk

LGTM - a couple suggestions but nothing critical

noahfalk · 2019-04-10T00:35:48Z

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/DiagnosticCounter.cs


        // arbitrarily we use name as the lock object.  
-        internal object MyLock { get { return _name; } }
+        protected object MyLock { get { return Name; } }


Did we need to change this from internal -> protected? I assume it isn't exposed via the reference assembly, but if we can mark it 'internal' that makes it a little more obvious that we haven't exposed it to customers.

noahfalk · 2019-04-10T00:48:10Z

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/EventCounter.cs

        }

-        protected void Enqueue(float value)
+        protected void Enqueue(double value)


Could we make this private? I don't see any type that derives from EventCounter so I assume no code outside of this class uses this member?

* rename BaseCounter to DiagnosticCounter * Change MetaData->Metadata * Make EventSource and Name a property for counter classes * Make the counter APIs public * fix build errors * Change float to double * Few cleanups, fix test * fix GetMetadataString * PR feedback * More PR feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

davidfowl · 2019-04-18T01:52:18Z

@sywhang Did we expose these in a reference assembly?

cc @noahfalk

sywhang · 2019-04-18T02:16:06Z

Do you mean in System.Diagnostics.Tracing.dll? In that case, no. I moved EventCounter out of there to CoreLib and all the new Counter APIs are in CoreLib as well.

I don't think these changes shipped officially yet because it got merged after the preview 4 snap date. So far I've been testing these by creating them using reflection on System.Private.CoreLib.dll.

davidfowl · 2019-04-18T03:30:44Z

Ah! When will it be consumable in a public API? I’d like to see if we can get a requests per second counter going

noahfalk · 2019-04-18T05:13:41Z

@sywhang - In order for customers to be able to access the new APIs we need to update the reference assembly in CoreFX and I don't think that happened yet? For example here is one Brian did to expose some other diagnostic APIs.

If you can get a PR up tomorrow I'd be glad to review it, the change shouldn't be controversial given the APIs were already approved.

sywhang · 2019-04-18T06:36:09Z

dotnet/corefx#36989 :-)

* rename BaseCounter to DiagnosticCounter * Change MetaData->Metadata * Make EventSource and Name a property for counter classes * Make the counter APIs public * fix build errors * Change float to double * Few cleanups, fix test * fix GetMetadataString * PR feedback * More PR feedback Commit migrated from dotnet/coreclr@95d37e0

sywhang added 6 commits April 8, 2019 03:03

rename BaseCounter to DiagnosticCounter

5f5eefe

Change MetaData->Metadata

49c15ab

Make EventSource and Name a property for counter classes

6ebb96a

Make the counter APIs public

2261a8d

fix build errors

3d2d207

Change float to double

b45b22f

Wraith2 reviewed Apr 8, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Diagnostics/Tracing/DiagnosticCounter.cs Outdated Show resolved Hide resolved

Wraith2 reviewed Apr 8, 2019

View reviewed changes

sywhang added 2 commits April 8, 2019 14:31

Few cleanups, fix test

437edf1

fix GetMetadataString

dc57c8a

sywhang requested review from jorive and noahfalk April 8, 2019 21:36