Add support for multiple readers #2596

utpilla · 2021-11-11T22:15:13Z

Fixes #2398

Changes

Add support for multiple readers

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

src/OpenTelemetry/ProviderExtensions.cs

codecov · 2021-11-11T22:25:37Z

Codecov Report

Merging #2596 (a2bc28f) into main (03313e7) will increase coverage by 0.35%.
The diff coverage is 80.56%.

@@            Coverage Diff             @@
##             main    #2596      +/-   ##
==========================================
+ Coverage   80.97%   81.33%   +0.35%     
==========================================
  Files         243      245       +2     
  Lines        8547     8614      +67     
==========================================
+ Hits         6921     7006      +85     
+ Misses       1626     1608      -18

Impacted Files	Coverage Δ
.../OpenTelemetry/Metrics/MeterProviderBuilderBase.cs	`100.00% <ø> (+2.32%)`	⬆️
src/OpenTelemetry/ProviderExtensions.cs	`45.45% <66.66%> (ø)`
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	`89.23% <72.85%> (-3.89%)`	⬇️
.../OpenTelemetry/Metrics/CompositeMetricReaderExt.cs	`73.91% <73.91%> (ø)`
src/OpenTelemetry/Metrics/MetricReaderExt.cs	`89.65% <89.65%> (ø)`
src/OpenTelemetry/Metrics/CompositeMetricReader.cs	`74.24% <100.00%> (+74.24%)`	⬆️
src/OpenTelemetry/Metrics/MetricReader.cs	`87.67% <100.00%> (-1.07%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`79.41% <0.00%> (-2.95%)`	⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`71.42% <0.00%> (-2.86%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️
... and 4 more

reyang · 2021-11-12T00:38:05Z

Is there a benchmark result?

utpilla · 2021-11-12T09:06:07Z

Is there a benchmark result?

// * Summary *

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1348 (21H1/May2021Update)
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=6.0.100
[Host] : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT
DefaultJob : .NET Core 3.1.21 (CoreCLR 4.700.21.51404, CoreFX 4.700.21.51508), X64 RyuJIT

MetricsBenchmarks original:

Method	AggregationTemporality	Mean	Error	StdDev	Allocated
CounterHotPath	Cumulative	36.96 ns	0.083 ns	0.078 ns	-
CounterWith1LabelsHotPath	Cumulative	100.17 ns	0.118 ns	0.104 ns	-
CounterWith3LabelsHotPath	Cumulative	443.49 ns	1.101 ns	1.030 ns	-
CounterWith5LabelsHotPath	Cumulative	630.47 ns	0.951 ns	0.794 ns	-
CounterWith6LabelsHotPath	Cumulative	736.86 ns	4.960 ns	4.639 ns	-
CounterWith7LabelsHotPath	Cumulative	869.27 ns	3.129 ns	2.927 ns	-
CounterHotPath	Delta	35.97 ns	0.110 ns	0.092 ns	-
CounterWith1LabelsHotPath	Delta	99.77 ns	0.211 ns	0.187 ns	-
CounterWith3LabelsHotPath	Delta	429.79 ns	0.616 ns	0.514 ns	-
CounterWith5LabelsHotPath	Delta	641.03 ns	1.080 ns	0.958 ns	-
CounterWith6LabelsHotPath	Delta	748.37 ns	1.739 ns	1.627 ns	-
CounterWith7LabelsHotPath	Delta	841.27 ns	2.278 ns	2.131 ns	-

MetricsBenchmarks with this PR:

Method	AggregationTemporality	Mean	Error	StdDev	Allocated
CounterHotPath	Cumulative	37.14 ns	0.047 ns	0.044 ns	-
CounterWith1LabelsHotPath	Cumulative	103.10 ns	0.123 ns	0.115 ns	-
CounterWith3LabelsHotPath	Cumulative	434.93 ns	1.645 ns	1.539 ns	-
CounterWith5LabelsHotPath	Cumulative	639.97 ns	6.215 ns	5.814 ns	-
CounterWith6LabelsHotPath	Cumulative	744.34 ns	5.044 ns	4.212 ns	-
CounterWith7LabelsHotPath	Cumulative	854.73 ns	16.145 ns	19.827 ns	-
CounterHotPath	Delta	37.19 ns	0.043 ns	0.040 ns	-
CounterWith1LabelsHotPath	Delta	99.42 ns	0.136 ns	0.120 ns	-
CounterWith3LabelsHotPath	Delta	440.10 ns	3.166 ns	2.961 ns	-
CounterWith5LabelsHotPath	Delta	633.21 ns	1.330 ns	1.039 ns	-
CounterWith6LabelsHotPath	Delta	735.38 ns	1.419 ns	1.327 ns	-
CounterWith7LabelsHotPath	Delta	841.17 ns	1.895 ns	1.680 ns	-

MetricCollectBenchmarks original:

Method	UseWithRef	Mean	Error	StdDev	Gen 0	Allocated
Collect	False	34.93 us	0.127 us	0.119 us	-	200 B
Collect	True	24.03 us	0.095 us	0.089 us	0.0305	200 B

MetricCollectBenchmarks with this PR:

Method	UseWithRef	Mean	Error	StdDev	Gen 0	Allocated
Collect	False	35.17 us	0.215 us	0.191 us	-	200 B
Collect	True	23.24 us	0.028 us	0.025 us	0.0305	200 B

MetricViewBenchmarks original:

Method	ViewConfig	Mean	Error	StdDev	Allocated
CounterMeasurementRecordingWithThreeTags	NoView	468.3 ns	1.91 ns	1.78 ns	-
CounterMeasurementRecordingWithThreeTags	ViewNoInstrSelect	463.0 ns	2.52 ns	2.24 ns	-
CounterMeasurementRecordingWithThreeTags	ViewSelectsInstr	573.3 ns	1.29 ns	1.20 ns	-

MetricViewBenchmarks with this PR:

Method	ViewConfig	Mean	Error	StdDev	Allocated
CounterMeasurementRecordingWithThreeTags	NoView	65.07 ns	0.112 ns	0.094 ns	-
CounterMeasurementRecordingWithThreeTags	ViewNoInstrSelect	65.25 ns	0.092 ns	0.086 ns	-
CounterMeasurementRecordingWithThreeTags	ViewSelectsInstr	64.91 ns	0.132 ns	0.124 ns	-

The numbers are significantly low for MetricViewBenchmarks as the benchmark code does not have any reader. With this PR, we do not enable measurements if there is no reader added to the MeterProviderSdk

src/OpenTelemetry/Metrics/MetricReader.cs

…or-MultipleReaders

src/OpenTelemetry/Metrics/CompositeMetricReader.cs

reyang

LGTM with some suggestions (doesn't have to block this PR).

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

CodeBlanch · 2021-11-18T17:57:01Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

-                    // TODO: Revisit to see if we need to do metrics.TrimExcess()
-                    var metrics = new List<Metric>(maxCountMetricsToBeCreated);
-                    lock (this.instrumentCreationLock)
+                    if (this.reader != null)


Is this.reader == null a valid case? Thinking maybe ctor should throw in that case so we don't need to check it.

Maybe it's fine. Similar discussion here #2476.

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

src/OpenTelemetry/Metrics/MetricReaderExt.cs

src/OpenTelemetry/Metrics/CompositeMetricReader.cs

cijothomas · 2021-11-18T20:55:39Z

src/OpenTelemetry/Metrics/CompositeMetricReader.cs

+            for (var cur = this.head; cur != null; cur = cur.Next)
+            {
+                var metric = cur.Value.AddMetricWithNoViews(instrument);
+                metrics.Add(metric);


add only if notnull

and in the provider, you can check the list.Count > 0 before enable measurement event is called.

I'm adding null to preserve the order in which we process the metrics when we have multiple readers. CompositeMetricReader checks for null before calling the reader to update the value.

In future, if we allow readers to have their own MaxMetrics limit, this would still work as we will know which readers need to update the value and which don't.

cijothomas · 2021-11-18T20:58:22Z

src/OpenTelemetry/Metrics/CompositeMetricReader.cs

+        internal List<Metric> AddMetricsWithNoViews(Instrument instrument)
+        {
+            var metrics = new List<Metric>();
+            for (var cur = this.head; cur != null; cur = cur.Next)


for a followup/todo - see if we need to catch exceptions from individual readers, so that anyone of them throwing won't affect the others.

We don't expect this method to throw any exceptions, right?

not sure. As a general principle, one reader's bad behaviour shouldn't affect other readers

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

cijothomas · 2021-11-18T21:15:14Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

            Debug.Assert(instrument != null, "instrument must be non-null.");
-            if (metric == null)
+
+            if (this.compositeMetricReader == null)


the name of the method no longer makes sense as it handles single stream(Metric) and multiple (List).
We should make 3 separate methods for Metric, List of Metic, List of (list of metric). Might save all the if-checks in hot path.
Good for a follow up to avoid making this PR bigger.

cijothomas · 2021-11-18T21:27:58Z

src/OpenTelemetry/Metrics/CompositeMetricReader.cs

+            for (var cur = this.head; cur != null; cur = cur.Next)
+            {
+                var metrics = cur.Value.AddMetricsListWithViews(instrument, metricStreamConfigs);
+                metricsSuperList.Add(metrics);


only add if the metric.Count > 0

Same as #2596 (comment)

cijothomas

Overall looks good. Left few comments.
Will do another round of review later today before merge

cijothomas

merging to make progress and pave way for others PR, potentially with merge conflicts!. Have few non-blocking items for follow up.

…hub.com/utpilla/opentelemetry-dotnet into utpilla/Add-Support-For-MultipleReaders

utpilla added 2 commits November 10, 2021 22:47

Add support for multiple readers

0b66c01

Add unit tests

1a70f55

utpilla requested a review from a team November 11, 2021 22:15

Update CHANGELOG.md

c6234f6

utpilla commented Nov 11, 2021

View reviewed changes

src/OpenTelemetry/ProviderExtensions.cs Outdated Show resolved Hide resolved

utpilla added 3 commits November 12, 2021 00:22

Fix MeterProviderSdk ctor when there is no reader added

22004e6

Update unit test for multiple readers to use ForceFlush

c217a3a

Fix unit test for no readers

93011e6

reyang reviewed Nov 12, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MetricReader.cs Outdated Show resolved Hide resolved

utpilla added 5 commits November 12, 2021 12:41

Update unit test for multiple readers to add gauge

6dc3bae

Merge remote-tracking branch 'origin/main' into utpilla/Add-Support-F…

9fb5b7d

…or-MultipleReaders

Merge with origin/main

da3b955

Update public API docs

b5be41b

Merge with origin/main

f428307

utpilla force-pushed the utpilla/Add-Support-For-MultipleReaders branch from f137aba to f428307 Compare November 18, 2021 06:01

Split MetricReader into partial classes

b5e568c

reyang reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/CompositeMetricReader.cs Outdated Show resolved Hide resolved

reyang approved these changes Nov 18, 2021

View reviewed changes

CodeBlanch reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MeterProviderSdk.cs Outdated Show resolved Hide resolved

CodeBlanch reviewed Nov 18, 2021

View reviewed changes

cijothomas reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MeterProviderSdk.cs Show resolved Hide resolved

cijothomas reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MetricReaderExt.cs Outdated Show resolved Hide resolved

cijothomas reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/CompositeMetricReader.cs Outdated Show resolved Hide resolved

cijothomas reviewed Nov 18, 2021

View reviewed changes

src/OpenTelemetry/Metrics/MeterProviderSdk.cs Show resolved Hide resolved

cijothomas reviewed Nov 18, 2021

View reviewed changes

Address PR comments

3b9fc45

cijothomas approved these changes Nov 18, 2021

View reviewed changes

utpilla and others added 5 commits November 18, 2021 16:03

Add comment about not enabling measurments

901bbe4

Merge branch 'main' into utpilla/Add-Support-For-MultipleReaders

bd61f39

Mark extension method internal

c8d5edf

Merge branch 'utpilla/Add-Support-For-MultipleReaders' of https://git…

3b17872

…hub.com/utpilla/opentelemetry-dotnet into utpilla/Add-Support-For-MultipleReaders

Update public API docs

a2bc28f

cijothomas merged commit ca29142 into open-telemetry:main Nov 19, 2021

cijothomas mentioned this pull request Nov 20, 2021

Stress test modified to have a reader to force sdk to do aggregation #2646

Closed

reyang mentioned this pull request Nov 24, 2021

Update the metrics example #2675

Merged

tomkerkhove mentioned this pull request Nov 25, 2021

InvalidOperationException not bubbled when using multiple metric readers through deferred builder #2586

Closed

utpilla deleted the utpilla/Add-Support-For-MultipleReaders branch November 3, 2023 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple readers #2596

Add support for multiple readers #2596

utpilla commented Nov 11, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading

reyang commented Nov 12, 2021

utpilla commented Nov 12, 2021

reyang left a comment

CodeBlanch Nov 18, 2021 •

edited

Loading

reyang Nov 18, 2021

cijothomas Nov 18, 2021

cijothomas Nov 18, 2021

utpilla Nov 18, 2021

cijothomas Nov 18, 2021

utpilla Nov 18, 2021

cijothomas Nov 18, 2021

cijothomas Nov 18, 2021

cijothomas Nov 18, 2021

utpilla Nov 18, 2021

cijothomas left a comment

cijothomas left a comment

Add support for multiple readers #2596

Add support for multiple readers #2596

Conversation

utpilla commented Nov 11, 2021 • edited Loading

Changes

codecov bot commented Nov 11, 2021 • edited Loading

Codecov Report

reyang commented Nov 12, 2021

utpilla commented Nov 12, 2021

reyang left a comment

Choose a reason for hiding this comment

CodeBlanch Nov 18, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

utpilla commented Nov 11, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading

CodeBlanch Nov 18, 2021 •

edited

Loading