Event counter listener #177

hananiel · 2022-01-20T20:17:55Z

Resolves #215

This PR provides the option to use any EventCounter as metric instrumentation, using an EventListener to "subscribe" to all Event sources the developer has specified.

The listener reads the event counter payload and passes it to the MeterTelemetryPublisher.

The publisher creates the instrumentation based on user provided definition or if none is provided, by using the information given by the event counter (see Inferring the Instrument Type).

The configuration is very easy. You can add all event counters of an event source just by

 options.AddEventSource("Microsoft-AspNetCore-Server-Kestrel")

or define each event counter fine grained by

options.AddEventSource("Microsoft-AspNetCore-Server-Kestrel")
 .With("connections-per-second", "The number of connections per update interval", InstrumentationType.Gauge)
 .With("total-connections", "The number of total connections", InstrumentationType.Gauge)

...OpenTelemetry.Contrib.EventCounterListener/OpenTelemetry.Contrib.EventCounterListener.csproj

src/OpenTelemetry.Contrib.EventCounterListener/EventCounterListener.cs

cijothomas · 2022-01-20T20:30:49Z

src/OpenTelemetry.Contrib.EventCounterListener/README.md

@@ -0,0 +1,58 @@
+# MySqlData Instrumentation for OpenTelemetry


cijothomas · 2022-01-20T20:32:44Z

src/OpenTelemetry.Contrib.EventCounterListener/EventCounterListener.cs

+        {
+            if (eventData == null)
+            {
+                throw new ArgumentNullException(nameof(eventData));


does anyone catch this?

cijothomas · 2022-01-20T20:41:47Z

src/OpenTelemetry.Contrib.EventCounterListener/README.md

+    }
+}
+```
+


the usage is not clear... could you describe with an example?

If you are looking for how configuration is done, I havent coded that up yet, just making sure what I have so far is in the ballpark. Using the extension method provided (AddEventCounterListener) adds the EventListener that then republishes counters via metricsAPI. For example running the program listing in the Readme.md above produces the following output:

I assumed all counters will be instrumented as guages

Since this requires aggregation in the collect interval, the appropriate one is last value

Export cpu-usage, Unit: CPU Usage, Meter: OpenTelemetry.Contrib.EventCounterListener/0.0.0.0 (2022-01-20T03:48:23.3248145Z, 2022-01-20T03:48:37.2994387Z] DoubleGauge Value: 0 Export working-set, Unit: Working Set, Meter: OpenTelemetry.Contrib.EventCounterListener/0.0.0.0 (2022-01-20T03:48:23.3275819Z, 2022-01-20T03:48:37.2994398Z] DoubleGauge Value: 31 Export gc-heap-size, Unit: GC Heap Size, Meter: OpenTelemetry.Contrib.EventCounterListener/0.0.0.0 (2022-01-20T03:48:23.3276834Z, 2022-01-20T03:48:37.2994400Z] DoubleGauge Value: 5 Export gen-0-gc-count, Unit: Gen 0 GC Count, Meter: OpenTelemetry.Contrib.EventCounterListener/0.0.0.0 (2022-01-20T03:48:23.3280650Z, 2022-01-20T03:48:37.2994402Z] DoubleGauge Value: 0

without seeing how a user would configure, its hard to comment on it.
Some qns:
What are the inputs provided by the user? Do they pick each and every eventcounter?
Given there are different types of EventCounters, do we let user let us know if they want it treated as counter/gauge/histogram?

(Ofcourse we don't want to solve it in 1st iteration, but want to get a feel of how would the final version look like )

What are the inputs provided by the user? Do they pick each and every eventcounter?

I was thinking they can filter by EventSource, and then get more granular at the particular counter.

Given there are different types of EventCounters, do we let user let us know if they want it treated as counter/gauge/histogram?

I have made a list of well known eventcounters and ran Reiley's "algorithm" for picking an instrument. In each case they made no sense to add them, since the eventCounters themselves are preaggregated, it makes no sense to further aggregate them - let me know if I am wrong about this or a different guide to pick instruments. Dependending on the answer to this, we could also make this the default option while giving users customizability to change this.

If you need to get on a zoom call for 15 mins, I am available today or next week is fine as well.

I did realize something I had misunderstood about different eventcounter types. I'll rework this.

cijothomas · 2022-01-21T15:52:49Z

NuGet.config

@@ -4,6 +4,8 @@
    <clear />
    <add key="NuGet" value="https://api.nuget.org/v3/index.json" />
    <add key="MyGet" value="https://www.myget.org/F/opentelemetry/api/v3/index.json" />
+    <add key="Dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" />


we can remove this, as we won't need any packages from non nuget.org/myget.org

twenzel · 2022-01-21T21:27:29Z

@hananiel Can we rename the projects OpenTelemetry.Contrib.EventCounterListener (+.Tests) to OpenTelemetry.Contrib.Instrumentation.EventCounterListener to be the same as the used namespace and as the other projects?

Fix metric name

Event source configuration

linux-foundation-easycla · 2022-01-24T17:55:32Z

✅ Hananiel Sarella (cf49964, 3ca132b, 0619d33)

❌ The commit (7381f48 ,79ce3c2c0b8804ec4c170101b835800f66134b73 ,769ff52fb03642ab88e621f93313535a7e9bb273 ,a92627b80af448febea13793bd9f8dfecdd9d5af ,3fb77d37110055af340f1391a2291311badf8c1d ,86367ca9143723170c0c4ff3bb3524ccc4032ab9 ,18c35078e8fddf3dab31890614e490208f193f88 ,bffd20016fd4e9458798a0419a84e0c59946798a ,69bd1025c990f3bdbaff9c3e87f25af4f00aadfa ,4f7ff3e3a2bd3787bc416cd5d5d9eed3e9111735 ,a56237593322511ca48cf198f721c65f65f57925) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

linux-foundation-easycla · 2022-01-24T19:47:49Z

✅ Hananiel Sarella (cf49964, 3ca132b, 0619d33)

❌ The commit (7381f48 ,79ce3c2c0b8804ec4c170101b835800f66134b73 ,769ff52fb03642ab88e621f93313535a7e9bb273 ,a92627b80af448febea13793bd9f8dfecdd9d5af ,3fb77d37110055af340f1391a2291311badf8c1d ,86367ca9143723170c0c4ff3bb3524ccc4032ab9 ,18c35078e8fddf3dab31890614e490208f193f88 ,bffd20016fd4e9458798a0419a84e0c59946798a ,69bd1025c990f3bdbaff9c3e87f25af4f00aadfa ,4f7ff3e3a2bd3787bc416cd5d5d9eed3e9111735 ,a56237593322511ca48cf198f721c65f65f57925) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

Test configuration via IConfiguration

twenzel · 2022-01-24T22:43:31Z

Hi @cijothomas 🙋🏼‍♂️,
we think the code is now fine for a review.
Please have look and tell us what you think.

cijothomas · 2022-02-01T16:21:47Z

src/OpenTelemetry.Contrib.Instrumentation.EventCounters/README.md

+}
+```
+
+Configuration can be done completely using the IConfiguration.


dont think we need to document the usage of IConfiguration or anything else here. Its not related to this package.

cijothomas · 2022-02-01T16:49:19Z

src/OpenTelemetry.Contrib.Instrumentation.EventCounters/README.md

+    {
+        public static void Main(string[] args)
+        {
+            using var meterprovider = Sdk.CreateMeterProviderBuilder()


could you describe the overall approach/design used in the PR description? Its a bit hard to figure out the design in a big PR without some writeup.

Event counter listener

Fix checks

Im5tu

Sorry it took me a few days to see the message @cijothomas - looks good, just a minor comment. Think we need to be aware that some EventSource's are available on startup as they are typically created on first use. So this could be before the EventListener has been created depending on the configuration of the app. Code as is will miss this if it's not fully initialized from what I can see. May be worth calling EventSource.GetSources() just after initialization (in our version we actually call this every 5-10 seconds to see if there's any new event sources that the user has request us track)

src/OpenTelemetry.Contrib.Instrumentation.EventCounters/Implementation/EventCounterListener.cs

cijothomas · 2022-02-08T01:28:08Z

src/OpenTelemetry.Contrib.Instrumentation.EventCounters/InstrumentationType.cs

+        /// Creates a counter instrumentation of long values which is an instrument that reports monotonically
+        /// increasing values.
+        /// </summary>
+        Counter = 0,


Counter is an sync instrument, so we should not use it from EventCounter.
Only supported instruments should be the ObservableCounter, ObservableGauge, and ObservableUpDownCounter (when that instrument is added)

It's only named "Counter". The publisher creates an ObservableCounter.
Should we rename the Counter enum value to ObservableCounter?

Yes. it's not clear which instrument from Metrics API should be used, so a matching name would help here.

The names were renamed.

Rename instrumentation types to reflect the created instrumentation

Fix test

twenzel · 2022-02-13T12:16:49Z

@cijothomas Anything else missing?

ZEXSM · 2022-02-27T19:34:20Z

@hananiel have you considered receiving diagnostic data via EventPipeSession, as is done by e.g. dotnet counters, instead of subscribing to an EventListener?

https://docs.microsoft.com/en-us/dotnet/core/diagnostics/eventpipe
https://docs.microsoft.com/en-us/dotnet/core/diagnostics/diagnostics-client-library

twenzel · 2022-02-28T18:42:07Z

@ZEXSM EventPipes should be used to retrieve the data for out-proc scenarios. EventListeners can be used for in-proc. Since we're running in-proc the EventListeners are the prefered way.

See https://docs.microsoft.com/en-us/dotnet/core/diagnostics/event-counters

cijothomas · 2022-03-03T02:41:27Z

@hananiel @twenzel To unblock and make progress, I'd suggest to break this down into smaller PRs, similar to suggestion for runtime metrics PR (#207 (comment)).

For this one, consider following

Add project/workflow
Extension method to enable, with just a basic support of specifying list of EventSource/EventCounter name. Default to ObservableGauge for all.
Add usage and example
Add capability to add allow user to specify which Instrument to use for each eventcounter.
Release 1st alpha and iterate based on initial feedback.

(its generally very hard to get people to review big PRs, so I'd strongly suggest small PRs, each addressing a specific concern, to constantly make progress, and hopefully attract more reviewers.)

hananiel · 2022-05-31T13:58:49Z

Closing in lieu of #365

hananiel added 2 commits January 20, 2022 14:45

Initial commit

cf49964

formatting

3ca132b

cijothomas reviewed Jan 20, 2022

View reviewed changes

...OpenTelemetry.Contrib.EventCounterListener/OpenTelemetry.Contrib.EventCounterListener.csproj Outdated Show resolved Hide resolved

cijothomas reviewed Jan 20, 2022

View reviewed changes

src/OpenTelemetry.Contrib.EventCounterListener/EventCounterListener.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jan 20, 2022

View reviewed changes

cijothomas reviewed Jan 21, 2022

View reviewed changes

twenzel and others added 12 commits January 21, 2022 22:49

Add missing headers

7381f48

Remove not required packages

79ce3c2

Add example

769ff52

Rename project

a92627b

First implementation for options

3fb77d3

Working metrics

86367ca

Update readme

18c3507

Fix metric name

Distinguish between different counters

bffd200

Better event source configuration

69bd102

Support more metric types

4f7ff3e

Add tests

a562375

Merge pull request #3 from twenzel/EventSourceConfiguration

0619d33

Event source configuration

twenzel and others added 4 commits January 24, 2022 21:24

Test configuration via IConfiguration

6dbbf45

Add NET 6 to PR build

1a9c341

Create package-Instrumentation.EventCounters.yml

d5abbfe

Merge pull request #4 from twenzel/EventSourceConfiguration

464dfe4

Test configuration via IConfiguration

hananiel added 2 commits January 24, 2022 19:15

Revert example to Net5.0

2e9350c

Markdown lint

a4e5b62

hananiel force-pushed the EventCounterListener branch from 837be0b to cfb4d0f Compare January 26, 2022 21:27

cijothomas reviewed Feb 1, 2022

View reviewed changes

cijothomas mentioned this pull request Feb 2, 2022

EventCounter API feedback open-telemetry/opentelemetry-dotnet#1502

Closed

twenzel and others added 10 commits February 4, 2022 23:20

Add more tests

07ac588

Use new Instrumentation type enum

7f5bf0d

Better way to infer instrumentation type

35d6a6a

Fix markdown lint

d88c972

Fix merge issue

6074cf1

Merge pull request #10 from twenzel/EventCounterListener

c98619b

Event counter listener

Fix sanity check

85530dd

Add more tests

1f66660

Rename variables

188b919

Merge pull request #12 from twenzel/EventCounterListener

b3a7d27

Fix checks

Im5tu reviewed Feb 5, 2022

View reviewed changes

src/OpenTelemetry.Contrib.Instrumentation.EventCounters/Implementation/EventCounterListener.cs Show resolved Hide resolved

cijothomas reviewed Feb 8, 2022

View reviewed changes

reyang mentioned this pull request Feb 8, 2022

Provide .NET Runtime instrumentation libraries #204

Closed

Rename instrumentation types to reflect the created instrumentation

16ceae7

twenzel mentioned this pull request Feb 8, 2022

Rename instrumentation types to reflect the created instrumentation hananiel/opentelemetry-dotnet-contrib#14

Merged

Hananiel Sarella and others added 3 commits February 8, 2022 15:51

Merge pull request #14 from twenzel/EventCounterListener

bb3a3d4

Rename instrumentation types to reflect the created instrumentation

Fix test

02f3e19

Merge pull request #15 from twenzel/EventCounterListener

a32c005

Fix test

hananiel mentioned this pull request Feb 18, 2022

Runtime instrumentation #207

Merged

hananiel mentioned this pull request May 18, 2022

Create an EventCounter Listener that subscribes to all event counters #365

Merged

hananiel closed this May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event counter listener #177

Event counter listener #177

hananiel commented Jan 20, 2022 •

edited

Loading

cijothomas Jan 20, 2022

cijothomas Jan 20, 2022

cijothomas Jan 20, 2022

hananiel Jan 21, 2022

cijothomas Jan 21, 2022

hananiel Jan 21, 2022

hananiel Jan 23, 2022

cijothomas Jan 21, 2022

hananiel Jan 24, 2022

twenzel commented Jan 21, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jan 24, 2022

linux-foundation-easycla bot commented Jan 24, 2022

twenzel commented Jan 24, 2022

cijothomas Feb 1, 2022

cijothomas Feb 1, 2022

Im5tu left a comment •

edited

Loading

cijothomas Feb 8, 2022

twenzel Feb 8, 2022

cijothomas Feb 8, 2022

twenzel Feb 8, 2022

twenzel commented Feb 13, 2022

ZEXSM commented Feb 27, 2022

twenzel commented Feb 28, 2022

cijothomas commented Mar 3, 2022

hananiel commented May 31, 2022

		@@ -0,0 +1,58 @@
		# MySqlData Instrumentation for OpenTelemetry

Event counter listener #177

Event counter listener #177

Conversation

hananiel commented Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twenzel commented Jan 21, 2022 • edited Loading

linux-foundation-easycla bot commented Jan 24, 2022

linux-foundation-easycla bot commented Jan 24, 2022

twenzel commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Im5tu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twenzel commented Feb 13, 2022

ZEXSM commented Feb 27, 2022

twenzel commented Feb 28, 2022

cijothomas commented Mar 3, 2022

hananiel commented May 31, 2022

hananiel commented Jan 20, 2022 •

edited

Loading

twenzel commented Jan 21, 2022 •

edited

Loading

Im5tu left a comment •

edited

Loading