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

Have MeterContext::GetMeters return a copy instead of a span #1777

Closed
wants to merge 3 commits into from

Conversation

johanpel
Copy link
Contributor

@johanpel johanpel commented Nov 16, 2022

Fixes #1720

Changes

MeterContext::GetMeters returns a span viewing into the meters_ vector used to manage meter storage. While the span is constructed, the storage is locked, but when the span returns and the lock is released, another thread may modify the vector while the caller of MeterContext::GetMeters continues to work with the span.

This PR changes MeterContext::GetMeters to simply return a copy of the metrics storage vector while it is being locked.

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 16, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: johanpel / name: Johan Peltenburg (8635aa9)

@johanpel johanpel marked this pull request as ready for review November 16, 2022 12:46
@johanpel johanpel requested a review from a team November 16, 2022 12:46
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #1777 (c4b04be) into main (d0571d8) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1777      +/-   ##
==========================================
- Coverage   85.79%   85.74%   -0.05%     
==========================================
  Files         171      171              
  Lines        5214     5214              
==========================================
- Hits         4473     4470       -3     
- Misses        741      744       +3     
Impacted Files Coverage Δ
sdk/src/metrics/meter_context.cc 89.66% <100.00%> (ø)
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️

@lalitb
Copy link
Member

lalitb commented Nov 16, 2022

Thanks for the PR @johanpel. Was thinking if we should instead change the storage container for Meters from vector to list, if that can avoid creating the copy during metric collection time. Something similar is done for storing multiple span processors here.

@lalitb
Copy link
Member

lalitb commented Nov 17, 2022

@johanpel - I tried different approach to fix this issue in #1783 to avoid doing copy of meters. My concern is, even though we copy vector of shared_ptr, it can be still significant overhead for large number of meters as it is performed during hot path of collection. Let me know if you see any issues in that approach?

@lalitb
Copy link
Member

lalitb commented Nov 18, 2022

Closing this as fix is delivered here #1783

@lalitb lalitb closed this Nov 18, 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.

Segfaults due to corrupted InstrumentationScope pointer when using multiple meters
2 participants