Skip to content

Fix #1663 Threading issue between Meter::RegisterSyncMetricStorage and Meter::Collect#1666

Merged
ThomsonTan merged 11 commits into
open-telemetry:mainfrom
lalitb:fix-race-meter
Oct 11, 2022
Merged

Fix #1663 Threading issue between Meter::RegisterSyncMetricStorage and Meter::Collect#1666
ThomsonTan merged 11 commits into
open-telemetry:mainfrom
lalitb:fix-race-meter

Conversation

@lalitb
Copy link
Copy Markdown
Member

@lalitb lalitb commented Oct 7, 2022

Fixes #1663

Changes

Add a lock guard to prevent race conditions arising due to

  • Concurrent creation of sync/async instruments.
  • Creation of sync/async instruments while Metric collection is ongoing.
  • Concurrent collection of metrics for same meters.

These operations should ensure that sync/sync storage structures and callback registry are atomically updated.
Also added unit-test to stress meter using multiple threads.

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

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team October 7, 2022 22:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 7, 2022

Codecov Report

Merging #1666 (5fc0812) into main (7140166) will increase coverage by 0.79%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
+ Coverage   84.97%   85.76%   +0.79%     
==========================================
  Files         170      170              
  Lines        5134     5137       +3     
==========================================
+ Hits         4362     4405      +43     
+ Misses        772      732      -40     
Impacted Files Coverage Δ
sdk/src/metrics/meter.cc 56.16% <100.00%> (+23.09%) ⬆️
sdk/src/metrics/state/temporal_metric_storage.cc 98.22% <0.00%> (+1.79%) ⬆️
sdk/src/metrics/async_instruments.cc 100.00% <0.00%> (+20.00%) ⬆️
sdk/src/metrics/state/observable_registry.cc 80.00% <0.00%> (+22.86%) ⬆️

@lalitb lalitb marked this pull request as draft October 8, 2022 07:44
@lalitb lalitb marked this pull request as ready for review October 10, 2022 03:02
@lalitb
Copy link
Copy Markdown
Member Author

lalitb commented Oct 10, 2022

This is ready for review now.

Copy link
Copy Markdown
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.
It would be great if you could make sure at least some of the threads finish in the unit test.
For example each thread can increment a counter when they start and decrement it when they finish.

Comment thread sdk/test/metrics/meter_test.cc
Comment thread sdk/test/metrics/meter_test.cc Outdated
Comment thread sdk/test/metrics/meter_test.cc Outdated
@lalitb
Copy link
Copy Markdown
Member Author

lalitb commented Oct 10, 2022

Thanks for the fix. It would be great if you could make sure at least some of the threads finish in the unit test. For example each thread can increment a counter when they start and decrement it when they finish.

Have added logic to make threads to join before finishing the unit test. Hope it suffices.

Comment thread sdk/src/metrics/meter.cc Outdated
@ThomsonTan ThomsonTan merged commit 99f2ba4 into open-telemetry:main Oct 11, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 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.

[Metrics SDK] Threading issue between Meter::RegisterSyncMetricStorage and Meter::Collect

3 participants