Initial Metric API/SDK framing #2034
Conversation
Codecov Report
@@ Coverage Diff @@
## metrics #2034 +/- ##
===========================================
+ Coverage 79.85% 82.54% +2.68%
===========================================
Files 203 205 +2
Lines 6498 6461 -37
===========================================
+ Hits 5189 5333 +144
+ Misses 1309 1128 -181
|
|
|
||
| namespace OpenTelemetry.Metrics | ||
| { | ||
| public class MetricProvider : IDisposable |
There was a problem hiding this comment.
This is different from my understanding, I guess:
- The name will be
MeterProvider. - It will derive from
OpenTelemetry.BaseProvider. - We might want to move something to the API? (e.g. considering if someone wants to implement their own SDK) @cijothomas might have better idea here.
There was a problem hiding this comment.
We can follow similar approach as tracing. Will iterate over several PRs. :)
| private BuildOptions options; | ||
| private ConcurrentDictionary<Meter, int> meters; | ||
| private MeterListener listener; | ||
| private CancellationTokenSource cts; |
There was a problem hiding this comment.
| private CancellationTokenSource cts; | |
| private readonly CancellationTokenSource cts; |
|
|
||
| public Meter GetMeter(string name, string version) | ||
| { | ||
| var meter = new Meter(name, version); |
There was a problem hiding this comment.
Might need some concurrency support here.
There was a problem hiding this comment.
For example, if the same name is already used, we could end up creating a Meter via .NET API but fail to update the this.meters dictionary.
If creating a dangling meter doesn't have any negative impact, it is probably fine.
There was a problem hiding this comment.
It is unspecified whether or under which conditions the same or different Meter instances are returned from this functions.
We might need to define the behavior for OpenTelemetry .NET?
|
|
||
| public bool Verbose { get; set; } = true; | ||
|
|
||
| public int ObservationPeriodMilliseconds { get; set; } = 1000; |
There was a problem hiding this comment.
Not a blocker, I think we need to call out that this is not observation period, instead, it is the delay/sleep between two observations.
Let's say each observation callback takes 800ms, and ObservationPeriodMilliseconds is 200ms, we will end up with pushes at every 1000ms (instead of every 200ms) approximately.
There was a problem hiding this comment.
Consider something similar to scheduledDelayMillis in the tracing spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#built-in-span-processors
There was a problem hiding this comment.
And this
Note that in OTel.NET it was called "Milliseconds" instead of "Millis" since the former is more idiomatic to .NET (and the later is more Java-ish).
Setting up initial framing for Metrics API and SDK based on currently available specs.