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

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

Merged
merged 11 commits into from
Oct 11, 2022

Conversation

lalitb
Copy link
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

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
Member Author

lalitb commented Oct 10, 2022

This is ready for review now.

Copy link
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.

sdk/test/metrics/meter_test.cc Show resolved Hide resolved
sdk/test/metrics/meter_test.cc Outdated Show resolved Hide resolved
@lalitb
Copy link
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.

@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