Refactor MeterProvider to be similar to TracerProvider #2141

cijothomas · 2021-07-15T23:03:37Z

Changes

Match MeterProvider more closely to TracerProvider, with most responsibilities pushed off elsewhere. MeterProvider just holds the configuration (processors, and views in the future..), just like tracerprovider.
Reuse most of the already existing classes like BaseExporter, and model MetricExporter similar to TraceExporter. This is iteration 1, I expect many more rounds to this in follow up PRs.
Add Push and Pull based exporter.
Removed console exporter from SDK project, and moved it off to Exporter.Console project. (like logs and traces)
The above forced the MetricItem and associated classes to be made public. Alternate option is to give internalsvisible to Console. Picked the former approach in this PR. The "MetricItem" interface is something we'll need to come back and modify based on feedback from OTLP Exporter work, and other exporters. It is also to be explored to avoid "MetricItem" class completely, and give the exporters access to the "aggregated in-memory stream" in a different way to avoid allocs.
Only one "pipeline" works. Though any # of MetricProcessors can be added, adding more than one would result in incorrect metrics. This is going to be fixed in the immediately upcoming PR.
The current code somewhat "shared" the aggregator for multiple exporters, if the exporters same exact same export interval. This PR breaks that capability. Will come back and see if this "shared" aggregator can be brought back. I think in the initial version, we'll have "duplicated" aggregators

Sorry about the long list of changes in single PR! All of this was interconnected, and I felt it was not worth the effort to split them. After this refactor, hoping to get very localized changes only :)

cijothomas · 2021-07-15T23:48:17Z

src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs

        /// <returns><see cref="MeterProvider"/>.</returns>
-        public static MeterProviderBuilder AddProcessor(this MeterProviderBuilder meterProviderBuilder, MeasurementProcessor processor)
+        public static MeterProviderBuilder AddMeasurementProcessor(this MeterProviderBuilder meterProviderBuilder, MeasurementProcessor processor)


this might be removed from the spec. if that happens, we'll also remove here.

If we need it, it might be SetMeasurementProcessor since we don't allow multiple processors.

cijothomas · 2021-07-15T23:49:46Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

@@ -138,43 +127,21 @@ protected override void Dispose(bool disposing)
            }
        }

-        private async Task CollectorTask(CancellationToken token, int collectionPeriodMilliseconds, MetricProcessor[] processors)
+        private MetricItem Collect()


MeterProvider has this method, but it never calls this. Its called by MetricProcessors, whenever they want. Typically in fixed time intervals (PushExporter) or in response to Scrape requests (PullExporter). Eitherway, MeterProvider doesn't care.

cijothomas · 2021-07-15T23:50:29Z

src/OpenTelemetry/Metrics/Processors/PullMetricProcessor.cs

+            this.getMetrics = getMetrics;
+        }
+
+        public void PullRequest()


Method called in response to some scrap request.

cijothomas · 2021-07-15T23:51:15Z

test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs

@@ -28,73 +28,6 @@ public class MetricApiTest
        [Fact]
        public void SimpleTest()
        {


removed as this was not doing any Tests. Will be adding unit tests for all features in subsequent PRs.

reyang · 2021-07-16T00:15:20Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

        private readonly CancellationTokenSource cts = new CancellationTokenSource();
        private readonly List<Task> collectorTasks = new List<Task>();
        private readonly MeterListener listener;
+        private readonly List<MeasurementProcessor> measurementProcessors = new List<MeasurementProcessor>();
+        private readonly List<MetricProcessor> metricProcessors = new List<MetricProcessor>();


Inside the SDK it might make sense to have one single MetricProcessor, which can be a CompositeProcessor (similar like what we did for log/trace). Not a blocker though.

yes! that's the plan. Will orchestrate it in such a way that one processor read the state, and provides it to the next ones.. Needs some non-trivial change, so making it in coming PRs.

reyang · 2021-07-16T00:16:48Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

-            this.MeasurementProcessors.AddRange(measurementProcessors);
-
-            this.ExportProcessors.AddRange(metricExportProcessors);
+            foreach (var processor in this.metricProcessors)


Remember the linked list?

It might save the foreach heap allocation.

yes. (this is at build time only, but yes will optimize)

reyang

LGTM.

codecov · 2021-07-16T00:19:04Z

Codecov Report

Merging #2141 (c3ff6f5) into metrics (9a5d9fd) will decrease coverage by 5.42%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           metrics    #2141      +/-   ##
===========================================
- Coverage    83.66%   78.24%   -5.43%     
===========================================
  Files          211      212       +1     
  Lines         6641     6632       -9     
===========================================
- Hits          5556     5189     -367     
- Misses        1085     1443     +358

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/AggregatorStore.cs	`0.00% <0.00%> (-88.41%)`	⬇️
src/OpenTelemetry/Metrics/InstrumentState.cs	`0.00% <0.00%> (-100.00%)`	⬇️
...elemetry/Metrics/MeterProviderBuilderExtensions.cs	`0.00% <0.00%> (-66.67%)`	⬇️
...c/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs	`0.00% <0.00%> (-92.00%)`	⬇️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	`0.00% <0.00%> (-97.27%)`	⬇️
src/OpenTelemetry/Metrics/Processors/MetricItem.cs	`0.00% <0.00%> (-100.00%)`	⬇️
...elemetry/Metrics/Processors/PullMetricProcessor.cs	`0.00% <0.00%> (ø)`
...elemetry/Metrics/Processors/PushMetricProcessor.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry.Api/Metrics/MeterProvider.cs	`0.00% <0.00%> (-100.00%)`	⬇️
...rc/OpenTelemetry/Metrics/DataPoint/DataValue{T}.cs	`0.00% <0.00%> (-100.00%)`	⬇️
... and 22 more

alanwest · 2021-07-16T01:11:28Z

src/OpenTelemetry/Metrics/Processors/MetricProcessor.cs

 namespace OpenTelemetry.Metrics
 {
    public abstract class MetricProcessor : BaseProcessor<MetricItem>
    {
+        // GetMetric or GetMemoryState or GetAggregatedMetrics..
+        // ...or some other names
+        public abstract void SetGetMetricFunction(Func<MetricItem> getMetrics);


I might not be thinking about things right, but would it make sense for a MetricProcessor to have a handle on the ParentProvider kinda like how we do for traces? That way instead of calling SetGetMetricFunction(this.Collect) in the MeterProviderSdk you'd have a handle on the provider to call Collect directly.

I like this idea. Will explore more on and try it in the subsequent PRs.

alanwest

These are some great improvements!

Refactor MeterProvider to be similar to TracerProvider

b2be7e7

cijothomas requested a review from a team July 15, 2021 23:03

cijothomas commented Jul 15, 2021

View reviewed changes

cijothomas requested a review from victlu July 15, 2021 23:53

cijothomas added 2 commits July 15, 2021 16:56

build fix

90421eb

buil fix again

c3ff6f5

reyang reviewed Jul 16, 2021

View reviewed changes

reyang approved these changes Jul 16, 2021

View reviewed changes

alanwest reviewed Jul 16, 2021

View reviewed changes

alanwest approved these changes Jul 16, 2021

View reviewed changes

cijothomas merged commit 63c28f0 into metrics Jul 16, 2021

cijothomas deleted the cijothomas/refactormetricsliketraces_part1 branch July 16, 2021 03:12

cijothomas mentioned this pull request Sep 2, 2021

Add Metric Reader and use IEnumerable for Metric collect instead of Batch #2306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MeterProvider to be similar to TracerProvider #2141

Refactor MeterProvider to be similar to TracerProvider #2141

cijothomas commented Jul 15, 2021

cijothomas Jul 15, 2021

reyang Jul 16, 2021

cijothomas Jul 15, 2021

cijothomas Jul 15, 2021

cijothomas Jul 15, 2021

reyang Jul 16, 2021

cijothomas Jul 16, 2021

reyang Jul 16, 2021

cijothomas Jul 16, 2021

reyang left a comment

codecov bot commented Jul 16, 2021

alanwest Jul 16, 2021

cijothomas Jul 16, 2021

alanwest left a comment

Refactor MeterProvider to be similar to TracerProvider #2141

Refactor MeterProvider to be similar to TracerProvider #2141

Conversation

cijothomas commented Jul 15, 2021

Changes

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

reyang left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest left a comment

Choose a reason for hiding this comment