Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delta exporters will Export only those points which received new update #2629

Merged

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Nov 17, 2021

Towards #2524 .
For delta, this only exports a point, if there is an update since last collection.
For cumulative, retains same behavior as before - once a point is updated, it'll be exported forever with same value.

This PR does not reclaim an inactive point for using when we are running out of metric points.

Perf impact:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1288 (21H1/May2021Update)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.100
[Host] : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT
DefaultJob : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT

Before:

Method AggregationTemporality Mean Error StdDev Allocated
CounterHotPath Cumulative 21.75 ns 0.363 ns 0.339 ns -
CounterWith1LabelsHotPath Cumulative 100.80 ns 1.198 ns 1.120 ns -
CounterWith3LabelsHotPath Cumulative 493.94 ns 9.660 ns 11.125 ns -
CounterWith5LabelsHotPath Cumulative 743.28 ns 12.839 ns 20.363 ns -
CounterWith6LabelsHotPath Cumulative 896.90 ns 14.747 ns 11.513 ns -
CounterWith7LabelsHotPath Cumulative 1,010.37 ns 18.028 ns 15.054 ns -
CounterHotPath Delta 19.32 ns 0.170 ns 0.133 ns -
CounterWith1LabelsHotPath Delta 102.44 ns 0.920 ns 0.768 ns -
CounterWith3LabelsHotPath Delta 500.14 ns 9.894 ns 11.778 ns -
CounterWith5LabelsHotPath Delta 751.90 ns 11.629 ns 11.942 ns -
CounterWith6LabelsHotPath Delta 870.99 ns 16.727 ns 17.897 ns -
CounterWith7LabelsHotPath Delta 995.99 ns 18.730 ns 23.003 ns -

With this PR:

Method AggregationTemporality Mean Error StdDev Allocated
CounterHotPath Cumulative 20.44 ns 0.395 ns 0.422 ns -
CounterWith1LabelsHotPath Cumulative 102.18 ns 1.442 ns 1.349 ns -
CounterWith3LabelsHotPath Cumulative 515.90 ns 9.947 ns 9.305 ns -
CounterWith5LabelsHotPath Cumulative 760.88 ns 14.142 ns 12.536 ns -
CounterWith6LabelsHotPath Cumulative 891.03 ns 10.911 ns 9.111 ns -
CounterWith7LabelsHotPath Cumulative 1,044.78 ns 9.312 ns 8.711 ns -
CounterHotPath Delta 19.89 ns 0.344 ns 0.354 ns -
CounterWith1LabelsHotPath Delta 102.00 ns 0.646 ns 0.573 ns -
CounterWith3LabelsHotPath Delta 481.05 ns 8.155 ns 7.230 ns -
CounterWith5LabelsHotPath Delta 759.45 ns 11.807 ns 11.044 ns -
CounterWith6LabelsHotPath Delta 881.06 ns 11.320 ns 10.035 ns -
CounterWith7LabelsHotPath Delta 1,037.07 ns 20.667 ns 26.873 ns -

(there is room for optimization to avoid the Delta vs Cumulative check on every Collect, as temporality is fixed at ctor itself. Good candidate for a small follow up PR)

Stress test:
Main:
Running (concurrency = 8), press to stop...
Stopping the stress test...

  • Total Loops: 216,780,099
  • Average Loops/Second: 7,037,858
  • Average CPU Cycles/Loop: 3,953

With this PR:
Stopping the stress test...

  • Total Loops: 204,110,887
  • Average Loops/Second: 6,808,235
  • Average CPU Cycles/Loop: 3,942

@cijothomas cijothomas requested a review from a team November 17, 2021 16:10
@cijothomas
Copy link
Member Author

This is almost same as what @alanwest did the Metric Point clean up PR, except that this PR does not do the actual clean up/reuse.

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #2629 (f95f2af) into main (5325185) will increase coverage by 0.01%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2629      +/-   ##
==========================================
+ Coverage   79.83%   79.84%   +0.01%     
==========================================
  Files         249      249              
  Lines        8627     8659      +32     
==========================================
+ Hits         6887     6914      +27     
- Misses       1740     1745       +5     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/MetricPoint.cs 90.75% <72.22%> (-4.30%) ⬇️
src/OpenTelemetry/Metrics/AggregatorStore.cs 86.80% <100.00%> (+1.19%) ⬆️
src/OpenTelemetry/Metrics/BatchMetricPoint.cs 84.61% <100.00%> (+0.61%) ⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs 93.03% <100.00%> (+0.04%) ⬆️
src/OpenTelemetry/Metrics/Metric.cs 95.77% <100.00%> (-0.06%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 96.87% <0.00%> (-0.79%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+5.88%) ⬆️

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Consider updating the changelog.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@cijothomas cijothomas merged commit ac98506 into open-telemetry:main Nov 17, 2021
@cijothomas cijothomas deleted the cijothomas/noexport_inactivepoints branch November 17, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants