Refactor aggregator and exporters #2295

cijothomas · 2021-08-31T07:15:43Z

Introduce Metric and MetricPoint, where Metric is a stream of aggregated metrics, containing upto N MetricPoints.
Each instrument result in one Metric.
(Once views are in, each instrument may result in more than on Metric. Also view config will be "hard-wired" at instrument creation time, so hot path has a simple Metric.Update() with no view lookups)
Each Metric pre-allocates MetricPoint[2000], to store time-series. The Dictionary is still used for lookup, to obtain the index within this array.
SDK caps max number of Metric (1000), and number of MetricPoints within each Metric to be 2000 - just hardcoded numbers
for now. This needs to be revisited to have a reasonable defaults, as we don't anticipate exposing this to user (unless spec asks so, but that'd likely be a separate discussion)
Exporters get Batch, reusing Batch from trace/log. This might be reconsidered for a simple IEnumerable.
Metric exposes GetDataPoints(), returning BatchMetricPoint. BatchMetricPoint is newly introduced as a way to iterate though the MetricPoint array, which is a struct and hence cannot re-use Batch. This need revisit based on 5 below.
MetricPoint is a struct now storing all type of Aggregator output - Sum,Gauge,Histogram. (Might save some cost by leveraging Union approach.)
There is cost involved in "copying" the struct when iterating. Will explore better ways to handle this.
All exporters modified with the new Metric/MetricPoint structure. This fixes the Prometheus exporter issue, and optimizes OTLP as well. The histogram is also modelled as per OTLP which is more efficient. (Also fixed the bound issue which was reported by Alan)

// Upcoming in separate PRs

Explore if ConcurrentDictionary could be leveraged instead of Dictionary with locks.
Sorting of Tag keys/values is a major cost on hot path. Might be good to store the 1st seen orders to the Dictionary along with sorted, so if user keep reusing the same order, we can avoid the Sort cost. (costs additional memory, but likely justifiable)

Its a big PR, I can split to separate steps if needed. Or keep this as is, and address key issues as separate PRs.

codecov · 2021-09-01T18:34:06Z

Codecov Report

Merging #2295 (fbc8992) into metrics (94c6e56) will increase coverage by 4.31%.
The diff coverage is 59.45%.

@@             Coverage Diff             @@
##           metrics    #2295      +/-   ##
===========================================
+ Coverage    74.58%   78.90%   +4.31%     
===========================================
  Files          218      235      +17     
  Lines         6957     7476     +519     
===========================================
+ Hits          5189     5899     +710     
+ Misses        1768     1577     -191

Impacted Files	Coverage Δ
...InMemory/InMemoryExporterMetricHelperExtensions.cs	`0.00% <0.00%> (ø)`
...metry.Exporter.InMemory/InMemoryExporterOptions.cs	`0.00% <0.00%> (ø)`
...etryProtocol/OtlpMetricExporterHelperExtensions.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry/Logs/LogRecord.cs	`93.84% <ø> (ø)`
...enTelemetry/Logs/OpenTelemetryLoggingExtensions.cs	`85.71% <ø> (ø)`
src/OpenTelemetry/Metrics/DataPoint/DataValue.cs	`0.00% <ø> (ø)`
...elemetry/Metrics/MeterProviderBuilderExtensions.cs	`77.77% <ø> (+77.77%)`	⬆️
...Metrics/MetricAggregators/GaugeMetricAggregator.cs	`0.00% <0.00%> (ø)`
...ics/MetricAggregators/SumMetricAggregatorDouble.cs	`0.00% <0.00%> (ø)`
...trics/MetricAggregators/SumMetricAggregatorLong.cs	`0.00% <0.00%> (ø)`
... and 94 more

reyang · 2021-09-01T22:18:12Z

src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs

@@ -32,7 +32,7 @@ public ConsoleMetricExporter(ConsoleExporterOptions options)
        {
        }

-        public override ExportResult Export(in Batch<MetricItem> batch)
+        public override ExportResult Export(in Batch<Metric> batch)


I think 1) metrics exporter will always run in a separate thread (unlike trace/log exporter which might run in the hot path - e.g. SimpleExportingProcessor) 2) metrics exporter will only be triggered at a much lower frequency (unlike trace/log exporter which might get triggered for every single event), so it might be actually easier if we define this as IEnumerable<Metric>.

I understand that there might be a desire to have ConsoleExporter<T> which covers logs/metrics/traces (for sake of consistency).

@CodeBlanch do you have strong opinion on this? (look at the change on the src/OpenTelemetry/Batch.cs, do you think the extra if-condition could slow down traces/logs?)

Yes agree to make this simpler with just IEnumerable, so we won't affect the other signals's hot path.

Could we do IList<T> instead of IEnumerable<T>? With that we could at least do a for (int i = 0; i < list.Count; i++) { var item = list[i]; } type of loop that doesn't allocate an enumerator. Or we could even pass the array directly. Array doesn't have a struct enumerator but AFAIK the JIT is smart enough to re-write a foreach loop on an array to be allocation-free.

I don't know if we will be able to tell the count of metrics or not, if we could, IList definitely works better.

I guess array won't work since it assumes certain memory layout. For example, if we pre-allocate memory and keep reusing them, we would need to compact the pre-allocated aggregation buffer before exporting (and that seems to be expensive).

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2306/files#diff-4d53a254dbe35c9d4bcfd7b6523549ab07b5372f3493d79b76797c724720cf75R170

I added a comment about this in the follow up PR.

We could also pass a ReadOnlySpan over the array?

reyang · 2021-09-01T22:22:18Z

src/OpenTelemetry/Batch.cs


-                if (circularBuffer == null)
+                if (circularBuffer == null && metrics == null)


I'm slightly concerned about the extra perf cost although it might be just 2 nanoseconds, as this will contribute to every single export call. https://github.com/open-telemetry/opentelemetry-dotnet/pull/2295/files#r700611706

Probably need some perf numbers.

See the final decision here: https://github.com/open-telemetry/opentelemetry-dotnet/pull/2327/files#r704879293

reyang · 2021-09-01T22:24:33Z

src/OpenTelemetry/Metrics/AggregationType.cs

+        /// <summary>
+        /// Calculate SUM from incoming delta measurements.
+        /// </summary>
+        DoubleSumIncomingDelta = 2,


Do we consider making this Flags?

e.g.

first four bits:

0001: Sum

0010: Gauge

0100: Histogram

anything else: Invalid

next two bits:

00: Invalid

01: Delta

10: Cumulative

11: Invalid

next four bits:

0001: int64 (long)

0010: double

anything else: Invalid

Yes. This in internal only enum, and this micro-optimization can be added in the future transparently.

reyang · 2021-09-01T23:17:27Z

I don't know how should we proceed on this big PR, want to get some input from @alanwest @CodeBlanch @utpilla:

I hope that we avoid merging something that touches the stable code, and later remove it - that makes the git blame and history very dirty. (e.g. if we decided not to touch the Batch.cs file, I think we SHOULD NOT leave a change history on it).
I think the general movement to reduce unnecessary memory allocation is great, and we should get the skeleton in place so other folks can help to divide & conquer.
We can also take the costly approach by splitting this in to a dozen PRs (like Refactor exporter - step 1 #1078), I don't know how much extra cost (@cijothomas do you have a wild guess?) that would introduce, and it doesn't seem to address 1).

I guess one possible way is that we merge this PR to the metrics branch, and perform clean up + refactor, and squash merge it back to main when we have good confidence?

cijothomas · 2021-09-01T23:32:39Z

I don't know how should we proceed on this big PR, want to get some input from @alanwest @CodeBlanch @utpilla:

I hope that we avoid merging something that touches the stable code, and later remove it - that makes the git blame and history very dirty. (e.g. if we decided not to touch the Batch.cs file, I think we SHOULD NOT leave a change history on it).

I think the general movement to reduce unnecessary memory allocation is great, and we should get the skeleton in place so other folks can help to divide & conquer.

We can also take the costly approach by splitting this in to a dozen PRs (like Refactor exporter - step 1 #1078), I don't know how much extra cost (@cijothomas do you have a wild guess?) that would introduce, and it doesn't seem to address 1).

I guess one possible way is that we merge this PR to the metrics branch, and perform clean up + refactor, and squash merge it back to main when we have good confidence?

I agree its best to leave the Batch<> untouched in this PR, given its unlikely to be the final stage we want.
Yes we can use metrics branch (we still kept metrics branch for any major works, and this would fit that bill). This would be easier than splitting.

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs

reyang · 2021-09-01T23:44:49Z

Yes we can use metrics branch (we still kept metrics branch for any major works, and this would fit that bill). This would be easier than splitting.

I'll approve the PR (knowing that there are multiple things that we can / probably should refactor / improve) if you could update it to target metrics branch (or whatever feature branch).

alanwest · 2021-09-01T23:47:04Z

src/OpenTelemetry/Metrics/MeterProviderSdk.cs

            foreach (var processor in this.metricProcessors)
            {
                processor.SetGetMetricFunction(this.Collect);
                processor.SetParentProvider(this);
+                temporality = processor.GetAggregationTemporality();


Kinda outside the scope of this PR, but would it make sense to do away with the list of MetricProcessors here since there can only be one? My first glance of this code made me think the last processor would dictate the temporality, but then I remembered:

opentelemetry-dotnet/src/OpenTelemetry/Metrics/MeterProviderBuilderSdk.cs

Lines 71 to 76 in 07114a0

internal MeterProviderBuilderSdk AddMetricProcessor(MetricProcessor processor)

{

if (this.MetricProcessors.Count >= 1)

{

throw new InvalidOperationException("Only one MetricProcessor is allowed.");

}

Yes, its planned for the next milestone , where we match the spec. (There is no MetricProcessor anymore :))
https://github.com/open-telemetry/opentelemetry-dotnet/milestone/27

alanwest · 2021-09-01T23:57:07Z

I support approving and targeting the metrics branch. @cijothomas, you mentioned doing another alpha release this Friday. Would we do the release from the metrics branch in this case?

alanwest · 2021-09-02T00:24:17Z

src/OpenTelemetry/Metrics/AggregationType.cs

+        /// <summary>
+        /// Calculate SUM from incoming delta measurements.
+        /// </summary>
+        LongSumIncomingDelta = 0,


What does Incoming mean in this context?

Incoming = Measurement. i.e the one reported by user using the counter API.

For sync counter, the measurement reported by the user is deltas.
For async counter, the measurement reported by the user is cumulative.

alanwest · 2021-09-02T00:26:04Z

src/OpenTelemetry/Metrics/AggregationType.cs

+        /// <summary>
+        /// Histogram.
+        /// </summary>
+        Histogram = 6,


Will there be the need for HistogramDelta and HistogramCumulative?

My 🧠 is still churning to grok what's going on here... so looks like LongSumIncomingDelta vs. LongSumIncomingCumulative, for example, really just speaks to whether whether the instrument is sync (delta) vs async (cumulative)... not so much the resultant aggregation temporality that is used?

you are right. This class simply refers to the types of aggregation we need to do internally. And speaks nothing about the outputted temporality!

alanwest · 2021-09-02T00:51:01Z

src/OpenTelemetry/Metrics/MetricPoint.cs

+                            initValue = this.doubleVal;
+                            newValue = initValue + number;
+                        }
+                        while (initValue != Interlocked.CompareExchange(ref this.doubleVal, newValue, initValue));


Thanks! 😄 I learned something here... Interlocked.Add equivalent for double

it has scope for improvement. I think we need to put a hard limit like this:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Internal/CircularBuffer.cs#L149-L152

alanwest · 2021-09-02T01:02:20Z

src/OpenTelemetry/Metrics/Metric.cs

+            this.Description = instrument.Description;
+            this.Unit = instrument.Unit;
+            this.Meter = instrument.Meter;
+            AggregationType aggType = default;


Probably for another PR, but I assume we might want some logging in the event none of the conditions match the instrument like Counter<Decimal>, for example. Or is this already prevented by .NET?

I'll check again, but i think .NET already restricts the types for

cijothomas · 2021-09-02T02:29:57Z

I support approving and targeting the metrics branch. @cijothomas, you mentioned doing another alpha release this Friday. Would we do the release from the metrics branch in this case?

The next release is tied to the exporter/aggregator improvement. That milestone(alpha3) will not happen until these changes are brought to main. Hopefully I can quickly address issues and take it to main soon.

I changed the PR to target metrics branch.

reyang

LGTM based on this discussion.

cijothomas · 2021-09-02T17:01:07Z

merging to metrics branch. Will send follow ups to the metrics branch, and do a merge to main once all concerns are addressed. The plan is to have alpha3 release include fixes (Promtheus fix, OTLP fix - histogram) from this PR.

ThomsonTan · 2021-09-02T17:15:13Z

src/OpenTelemetry/Batch.cs

            this.circularBuffer = circularBuffer ?? throw new ArgumentNullException(nameof(circularBuffer));
            this.targetCount = circularBuffer.RemovedCount + Math.Min(maxSize, circularBuffer.Count);
        }

+        internal Batch(T[] metrics, int maxSize)
+        {
+            Debug.Assert(maxSize > 0, $"{nameof(maxSize)} should be a positive number.");


Do we still need some runtime check on maxSize, as Debug.Assert doesn't show up in release build?

…y#2295

cijothomas · 2021-09-10T20:30:14Z

Introduce Metric and MetricPoint, where Metric is a stream of aggregated metrics, containing upto N MetricPoints.
Each instrument result in one Metric.
(Once views are in, each instrument may result in more than on Metric. Also view config will be "hard-wired" at instrument creation time, so hot path has a simple Metric.Update() with no view lookups)

Each Metric pre-allocates MetricPoint[2000], to store time-series. The Dictionary is still used for lookup, to obtain the index within this array.
SDK caps max number of Metric (1000), and number of MetricPoints within each Metric to be 2000 - just hardcoded numbers
for now. This needs to be revisited to have a reasonable defaults, as we don't anticipate exposing this to user (unless spec asks so, but that'd likely be a separate discussion)

Exporters get Batch, reusing Batch from trace/log. This might be reconsidered for a simple IEnumerable. - Update: We are going to re-use Batch and is being worked on in this PR : https://github.com/open-telemetry/opentelemetry-dotnet/pull/2327/files

Metric exposes GetDataPoints(), returning BatchMetricPoint. BatchMetricPoint is newly introduced as a way to iterate though the MetricPoint array, which is a struct and hence cannot re-use Batch. This need revisit based on 5 below. - Update: We are continuing with this approach for now. See below as well.

MetricPoint is a struct now storing all type of Aggregator output - Sum,Gauge,Histogram. (Might save some cost by leveraging Union approach.)
There is cost involved in "copying" the struct when iterating. Will explore better ways to handle this. Update: The copy perf cost is fixed -Modify MetricPoint to avoid copy #2321 . The "Union" approach is not yet done and will be revisited for later release.

All exporters modified with the new Metric/MetricPoint structure. This fixes the Prometheus exporter issue, and optimizes OTLP as well. The histogram is also modelled as per OTLP which is more efficient. (Also fixed the bound issue which was reported by Alan)

// Upcoming in separate PRs

Explore if ConcurrentDictionary could be leveraged instead of Dictionary with locks.

Sorting of Tag keys/values is a major cost on hot path. Might be good to store the 1st seen orders to the Dictionary along with sorted, so if user keep reusing the same order, we can avoid the Sort cost. (costs additional memory, but likely justifiable)

Its a big PR, I can split to separate steps if needed. Or keep this as is, and address key issues as separate PRs.

cijothomas added 12 commits August 30, 2021 23:47

Refactor aggregator and exporters

b7cfe07

build fix

075f2cc

fix prometheus

15b98dc

fix format

a169c91

Merge branch 'main' into cijothomas/metricexporter_refactor

d8b5524

build fix

0edc3f0

Merge branch 'main' into cijothomas/metricexporter_refactor

71e0fef

Add histogram to metric point

5cefd33

fix

20d8831

console exporter historgram support

4fa5fdc

build fix

2063999

fix httpclient metric tests

85dbba7

cijothomas added 4 commits September 1, 2021 13:40

prometheus histogram fix

58aaf2d

unwanted doc change revert

83623dc

space

f190cfa

Merge branch 'main' into cijothomas/metricexporter_refactor

07114a0

cijothomas marked this pull request as ready for review September 1, 2021 20:55

cijothomas requested a review from a team September 1, 2021 20:55

reyang reviewed Sep 1, 2021

View reviewed changes

alanwest reviewed Sep 1, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs Show resolved Hide resolved

alanwest reviewed Sep 1, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/MetricItemExtensions.cs Show resolved Hide resolved

alanwest reviewed Sep 1, 2021

View reviewed changes

alanwest reviewed Sep 2, 2021

View reviewed changes

cijothomas changed the base branch from main to metrics September 2, 2021 02:20

Merge branch 'metrics' into cijothomas/metricexporter_refactor

e2690cd

cleanup

1b7323c

cijothomas changed the title ~~WIP - Refactor aggregator and exporters~~ Refactor aggregator and exporters Sep 2, 2021

cijothomas added 2 commits September 2, 2021 09:44

add todo for unknown instrument

8142a92

unwanted using

fbc8992

reyang approved these changes Sep 2, 2021

View reviewed changes

alanwest approved these changes Sep 2, 2021

View reviewed changes

cijothomas merged commit 91cde47 into open-telemetry:metrics Sep 2, 2021

cijothomas deleted the cijothomas/metricexporter_refactor branch September 2, 2021 17:01

ThomsonTan reviewed Sep 2, 2021

View reviewed changes

cijothomas mentioned this pull request Sep 2, 2021

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

Merged

alanwest added a commit to alanwest/opentelemetry-dotnet that referenced this pull request Sep 9, 2021

Use Batch for metric export - previously implemented in open-telemetr…

672a6bf

…y#2295

alanwest mentioned this pull request Sep 9, 2021

Revive using Batch for metric export #2327

Merged

cijothomas mentioned this pull request Sep 11, 2021

AggregatorStore to use concurentdictionary #2339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor aggregator and exporters #2295

Refactor aggregator and exporters #2295

cijothomas commented Aug 31, 2021 •

edited

Loading

codecov bot commented Sep 1, 2021 •

edited

Loading

reyang Sep 1, 2021 •

edited

Loading

cijothomas Sep 1, 2021

CodeBlanch Sep 2, 2021 •

edited

Loading

reyang Sep 3, 2021

cijothomas Sep 3, 2021

CodeBlanch Sep 3, 2021

reyang Sep 1, 2021 •

edited

Loading

cijothomas Sep 10, 2021

reyang Sep 1, 2021 •

edited

Loading

cijothomas Sep 1, 2021

reyang commented Sep 1, 2021 •

edited

Loading

cijothomas commented Sep 1, 2021

reyang commented Sep 1, 2021 •

edited

Loading

alanwest Sep 1, 2021 •

edited

Loading

cijothomas Sep 2, 2021

alanwest commented Sep 1, 2021

alanwest Sep 2, 2021

cijothomas Sep 2, 2021

alanwest Sep 2, 2021 •

edited

Loading

cijothomas Sep 2, 2021

alanwest Sep 2, 2021

cijothomas Sep 2, 2021

alanwest Sep 2, 2021 •

edited

Loading

cijothomas Sep 2, 2021

cijothomas commented Sep 2, 2021

reyang left a comment

cijothomas commented Sep 2, 2021

ThomsonTan Sep 2, 2021

cijothomas commented Sep 10, 2021


		if (circularBuffer == null)
		if (circularBuffer == null && metrics == null)

	internal MeterProviderBuilderSdk AddMetricProcessor(MetricProcessor processor)
	{
	if (this.MetricProcessors.Count >= 1)
	{
	throw new InvalidOperationException("Only one MetricProcessor is allowed.");
	}

Refactor aggregator and exporters #2295

Refactor aggregator and exporters #2295

Conversation

cijothomas commented Aug 31, 2021 • edited Loading

codecov bot commented Sep 1, 2021 • edited Loading

Codecov Report

reyang Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch Sep 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

Choose a reason for hiding this comment

reyang Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang commented Sep 1, 2021 • edited Loading

cijothomas commented Sep 1, 2021

reyang commented Sep 1, 2021 • edited Loading

alanwest Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest Sep 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

Choose a reason for hiding this comment

alanwest Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas commented Sep 2, 2021

reyang left a comment

Choose a reason for hiding this comment

cijothomas commented Sep 2, 2021

Choose a reason for hiding this comment

cijothomas commented Sep 10, 2021

cijothomas commented Aug 31, 2021 •

edited

Loading

codecov bot commented Sep 1, 2021 •

edited

Loading

reyang Sep 1, 2021 •

edited

Loading

CodeBlanch Sep 2, 2021 •

edited

Loading

reyang Sep 1, 2021 •

edited

Loading

reyang Sep 1, 2021 •

edited

Loading

reyang commented Sep 1, 2021 •

edited

Loading

reyang commented Sep 1, 2021 •

edited

Loading

alanwest Sep 1, 2021 •

edited

Loading

alanwest Sep 2, 2021 •

edited

Loading

alanwest Sep 2, 2021 •

edited

Loading