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

Lock-free updates for HistogramSumCount #2961

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Mar 2, 2022

Fixes #2957

Similar to #2951

Changes

  • Implement lock-free updates for HistogramSumCount

Stress Test Results

I used the follow view configuration in the stress test SDK setup to provide empty bounds for the Histogram which in turn applies HistogramSumCount aggregation:

.AddView(instrumentName: "TestHistogram", new ExplicitBucketHistogramConfiguration() { Boundaries = new double[] { } })

To better understand the perf improvement, we need to look at two scenarios:

  1. There is not much contention to update the same MetricPoint. This was tested by running the following in the Run method of stress test:
TestHistogram.Record(
           random.Next(MaxHistogramMeasurement),
           new("DimName1", DimensionValues[random.Next(0, ArraySize)]),
           new("DimName2", DimensionValues[random.Next(0, ArraySize)]),
           new("DimName3", DimensionValues[random.Next(0, ArraySize)]));
  1. There is high contention to update the same MetricPoint. This was tested by running the following in the Run method of stress test:
TestHistogram.Record(
           random.Next(MaxHistogramMeasurement),
           new("DimName1", "DimVal1"),
           new("DimName2", "DimVal2"),
           new("DimName3", "DimVal3"));

While there is a perf improvement in both the cases, there is a substantial improvement in the second case where there is high contention to update the same MetricPoint.

For the first scenario, Loops/ second go up from ~20M to ~24M (~20% increase)
For the second scenario, Loops/second go up from ~5.8M to ~8.3M (~43% increase)

Here are the numbers for the 1st and 2nd scenario respectively:

main branch

image

With this PR

image

@utpilla utpilla requested a review from a team March 2, 2022 02:39
@cijothomas
Copy link
Member

@utpilla could you resolve the conflicts and do combined changelog for both this and the previous one. The approach is good, and we can merge once CI is green.

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #2961 (872a829) into main (cee9006) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2961      +/-   ##
==========================================
- Coverage   84.01%   83.97%   -0.04%     
==========================================
  Files         254      254              
  Lines        8946     8950       +4     
==========================================
  Hits         7516     7516              
- Misses       1430     1434       +4     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/MetricPoint.cs 85.00% <75.00%> (-1.03%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 73.52% <0.00%> (-8.83%) ⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs 97.65% <0.00%> (+0.78%) ⬆️

@cijothomas
Copy link
Member

Merging. as this is doing same thing as another PR approved by all.

@cijothomas cijothomas merged commit af0c9b4 into open-telemetry:main Mar 4, 2022
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.

Implement lock-free updates for HistogramSumCount
4 participants