Design experiment #3

victlu · 2021-02-02T18:43:33Z

A PR for discussion. Not intended to be merged.

Currently, there are 3 different designs…

./examples3 – An interpretation for a simpler API/SDK (from Victor)
./experimental – An interpretation from a .NET perspective (From Noah)
./examples2 – Trying to implement an interpretation of existing OTel Metric Spec (From Victor)

New api sdk

victlu · 2021-02-02T18:48:29Z

experimental/ConsoleApp6/ConsoleApp6/Api.cs

+        public LabelSet DefaultLabels { get; }
+
+        public void Record(int measurement) { }
+        public void Record(int measurement, string labelName1, string labelValue1) { }


Looks like you're separating Key and Value. So, why not Record(..., string[] keys, string[] values)?

Sorry I completely missed some of these comments. My bad!
A few reasons:
a) It is one less allocation per metric recording (cost ~10ns)
b) passing strings at Record implies the names can be different every time which means the SDK has to search and/or sort them

RE: b. Although I agree, I don't think that's your top priority concern.

experimental/ConsoleApp6/ConsoleApp6/Api.cs

codecov-io · 2021-02-02T18:53:43Z

Codecov Report

❗ No coverage uploaded for pull request base (main@b212c91). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #3   +/-   ##
=======================================
  Coverage        ?   83.71%           
=======================================
  Files           ?      192           
  Lines           ?     6165           
  Branches        ?        0           
=======================================
  Hits            ?     5161           
  Misses          ?     1004           
  Partials        ?        0

victlu · 2021-02-02T18:55:00Z

experimental/ConsoleApp6/ConsoleApp6/Examples.cs

+{
+    class SquidAnalyzer
+    {
+        static readonly MetricSource metrics = new MetricSource("SquidLibrary",


Need to discuss if we really want to have a "Provider" as the starting point.

I think it makes more sense to just "new Counter(...)" immediately. And employ late binding to attach to a "provider".

victlu · 2021-02-02T18:57:55Z

experimental/ConsoleApp6/ConsoleApp6/Examples.cs

+                { "SquidBetaBuild", "true" },
+                { "SeafoodFeatureLevel", "4" }
+            });
+        static readonly Counter squidsAnalyzedCounter = metrics.CreateCounter("squids-analyzed");


I'm wondering if there's an easy way (preferably at compile time) to get the caller's fully qualified class name. That can act as the "default" name for this counter. It may be done with reflections (but it's slow).

Just like ILogger, we can also use generics to take a class name that we use for library name and we can get library version via reflection.

victlu · 2021-02-02T18:58:56Z

experimental/ConsoleApp6/ConsoleApp6/Examples.cs

+            string labelValue2 = "shrimp";
+            string labelValue3 = "clam";
+            string labelValue4 = "soup";
+            squidsAnalyzedCounter.Add(1,


This looks pretty awkward to me in passing in key/values...

victlu · 2021-02-02T19:02:09Z

experimental/design-notes.txt

+
+Micrometer (MM) - https://micrometer.io/docs/concepts
+
+1. Do the names that OpenTelemetry chose match industry conventions? 


I'm wondering maybe we leave the "naming" convention match to the SDK. Thus, API just needs to collect all the component fields, and let the SDK do what they want with forming a correct "name" for the exporter.

Basically, I'm saying maybe we are not so concern with naming from API side as it can be adjusted by the SDK.

victlu · 2021-02-02T19:03:50Z

experimental/design-notes.txt

+Micrometer (MM) - https://micrometer.io/docs/concepts
+
+1. Do the names that OpenTelemetry chose match industry conventions? 
+For the MM library there are different types of Meters such as timers, counters, gauges, etc. Depending on their type, meters implement operations such


I think I heard may calls for a "timing" type counter. Maybe we include a TimeRange counter of the form...

using (var timer = new Timer(...))
{
// Do Operations...
}

victlu · 2021-02-02T19:05:11Z

experimental/design-notes.txt

+As best I can tell the best conceptual mapping is that MM's Meter == OT's bound instrument. It doesn't worry me too much that OT would use a different
+name for the same concept, what worries me more is if OT is reusing the same names but altering the concept they apply to.
+
+2. MM has no equivalent abstraction for OT's Meter or Instrument. It goes straight from MeterProvider (MeterRegistry in MM-terminology) to a bound-instrument 


Agree. I would propose we employ "late binding" to attach a Provider.

victlu · 2021-02-02T19:59:59Z

experimental/design-notes.txt

+If MM has been succesful with these simpler abstractions it raises the question what value is OT getting from its more complex 4 layers of
+abstraction (MeterProvider->Meter->Instrument->BoundInstrument)?
+
+3. MM landed on a different set of instrument types than OT did. It would be good to understand why we believe OT's instrument set is the right set to pick?


Yeah, I think the OT SIG also pointed this out. I think Reiley will likely drive this to some better conclusion.

victlu · 2021-02-02T20:00:39Z

experimental/design-notes.txt

+or is there a scenario where the atomicity of the measurements matter?
+
+6. MM has a lot of API surface dedicated to configuring Meters (in MM terminology) and tags. I am guessing this is the kind of functionality that would be sitting in OT's
+Views API?


This will be a HUGE topic for SDK side. And probably will be very vendor opinionated in those discussions!

victlu · 2021-02-02T20:03:20Z

experimental/design-notes.txt

+Histogram     ValueRecorder
+NA            *Observer
+
+4. Prometheus uses Observe() API where OT uses Record() API. Prometheus doesn't appear to have asynchronous/callback style APIs.


I think we do need an "async" callback for reporting data. Or at least some examples of how to do this. Need to resolve the issue of Sum vs Incremental dataset. Cause any "Observed" value implies a Time Range, and thus, some kind of aggregation. This needs to be reviewed a bit deeper I think.

victlu · 2021-02-02T20:05:47Z

experimental/design-notes.txt

+var counter = new CounterOptions { Name = "my_counter" } // this defines the metric
+metrics.Measure.Counter.Increment(counter); // increments the value of the metric stored in this IMetrics
+
+3. Metrics are never bound. Tags can be provided at the time a measurement is captured and merged with tags set via global tags in the IMetrics interface


I'm wondering... Maybe we setup a memory storage for binding a set of KVs. Then, we just pass the id of that entry, instead of "binding" a meter. This way, any "bound" KVs can be shared by any meter and addressible with an id.

victlu · 2021-02-02T20:08:46Z

experimental/design-notes.txt

+Double and long. The typical distinction between counters, gauges, histograms, etc are defined separately as aggregations using the View API.
+3. Measures are created directly from a static factory method and they are not associated with any particular collection pipeline. A separate StatsRecorder object provides
+the data collection API.
+4. The StatsRecorder collection API builds up a list of measurements first, then commits it with a single API call. I didn't see any simpler option presented in the samples


We likely need a BatchRecord type functionality. To allow our user to inject data for the same "timeframe". I don't know what other scenarios requires "atomicity" (outside of marking same timeframe) but would like to find out more from others what the need here is.

example3/API/MetricSource.cs

The API review folks generally don't like to have interfaces with no members and we can do a type check that avoids needing them. If we want a better non-type based check I think the best approach will be to decide on a hinting API.

The perf regression from adding the T was larger than I remembered, about 15ns. I tracked it down to perf impact of making generic virtual calls. Unlike normal virtual calls that can be resolved by looking up the function pointer in a known slot in a table, the generic T requires a dictionary lookup in the runtime. Thankfully we can work around that by providing type specific overloads.

codecov-commenter · 2021-04-17T07:32:43Z

Codecov Report

❗ No coverage uploaded for pull request base (main@b212c91). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #3   +/-   ##
=======================================
  Coverage        ?   83.76%           
=======================================
  Files           ?      192           
  Lines           ?     6165           
  Branches        ?        0           
=======================================
  Hits            ?     5164           
  Misses          ?     1001           
  Partials        ?        0

After continued reflection I've reversed my earlier position on using object type for label values. I think most others wanted object and were just defering to me as the holdout so probably this isn't contentious? Also fixed a bug in the handling of 3 labeled instruments (hardcoded 2 at MeterInstrument.cs:165 should have been a 3)

…class

…elemetry-dotnet into design_experiment

victlu and others added 5 commits February 1, 2021 09:44

Example of a new API/SDK

fcb0110

API/SDK using abstract class

83bbd75

Merge pull request #2 from victlu/new-api-sdk

df91948

New api sdk

Setup more concurrent counters

19f30de

experimenting...

1c9c547

victlu commented Feb 2, 2021

View reviewed changes

experimental/ConsoleApp6/ConsoleApp6/Api.cs Outdated Show resolved Hide resolved

victlu commented Feb 2, 2021

View reviewed changes

experimental/ConsoleApp6/ConsoleApp6/Api.cs Show resolved Hide resolved

victlu commented Feb 2, 2021

View reviewed changes

victlu added 2 commits February 2, 2021 14:52

Introduce LabelSet

257ce4a

Try wrapping int/double into MetricValue struct

f4a2944

reyang reviewed Feb 3, 2021

View reviewed changes

example3/API/MetricSource.cs Outdated Show resolved Hide resolved

victlu added 7 commits February 3, 2021 11:41

Add Batch Record example. Add LabelHistogram example

c2b3c7b

Refactor based on discussion with Noah

3fec820

Create a MetricProvider as a top level.

f3ed245

Refactoring...

8e2826c

Churning...

b48dd2f

Add Hints to Meter

e94e258

Introduce a Queue for buffering incoming recordings

41d468f

victlu and others added 5 commits April 15, 2021 12:11

Hook up Api2 to our sdk

372c236

Add notification to API2

ae43b05

Add <T> to all instruments

a09e8c0

Removing the tagging interfaces

1a4de09

The API review folks generally don't like to have interfaces with no members and we can do a type check that avoids needing them. If we want a better non-type based check I think the best approach will be to decide on a hinting API.

victlu and others added 24 commits April 22, 2021 13:05

Target multiple frameworks

3f6f14e

Instroduce Metric API into OpenTelemetry

6631b93

Hook up MDM to see how it feels.

dc4c103

Get rid of UnboundMeterInstrument

375aec5

Remove MeasurementObserver and create ObservableMeterInstrument base …

5be6d5c

…class

Add description and unit

4673bba

Eliminate unneeded protected members

e647004

Eliminate build warnings

f53e597

Update MeterInstrumentListener API

3356527

Add Bench to compare cost of API/SDK vs IFx

1bc6bcd

Merge branch 'design_experiment' of https://github.com/noahfalk/opent…

b133580

…elemetry-dotnet into design_experiment

Update bench results

a0f3304

Fix bad checkin

61d4860

Remove strong name signing

80ad09c

Remove reference

c785586

Make it build!

e5ea7e0

Update to new Listener hookup

47ef1e3

Use IFxExtension to handle multi-dim labels

90a4e87

Setup Example to send to GenevaAgent

727dbf9

Try out passing KVP in bench

6b3f2c2

Not sure why CreateReadOnlySpan doesn't work!

c26e099

Get bench working for CreateReadOnlySpan

b0d08a7

Bench for CreateReadOnlySpan of 5 KVP

80e6ff7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design experiment #3

Design experiment #3

victlu commented Feb 2, 2021 •

edited

Loading

victlu Feb 2, 2021

noahfalk Mar 9, 2021

victlu Mar 9, 2021

codecov-io commented Feb 2, 2021 •

edited

Loading

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Mar 9, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

victlu Feb 2, 2021

codecov-commenter commented Apr 17, 2021


		Micrometer (MM) - https://micrometer.io/docs/concepts

		1. Do the names that OpenTelemetry chose match industry conventions?

Design experiment #3

Are you sure you want to change the base?

Design experiment #3

Conversation

victlu commented Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 2, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 17, 2021

Codecov Report

victlu commented Feb 2, 2021 •

edited

Loading

codecov-io commented Feb 2, 2021 •

edited

Loading