Fix error message caused by race condition when using PeriodicReader #1481
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1481 +/- ##
=====================================
Coverage 61.8% 61.8%
=====================================
Files 144 144
Lines 19810 19821 +11
=====================================
+ Hits 12256 12267 +11
Misses 7554 7554 ☔ View full report in Codecov by Sentry. |
|
@izquierdo The changes look to be in the right direction. Wondering if you plan to make this ready for review. Thanks. |
Yes, I haven't done so yet because of the new lines that are missing test coverage. Will mark it for review after updating the tests. Thank you. |
izquierdo
force-pushed
the
issue-1244
branch
2 times, most recently
from
e83f838
to
ebb7b7e
Compare
| let result = reader.collect(&mut rm); | ||
| result.expect_err("error expected when reader is not registered"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think it would be easy to add the test for success scenario (something like):
- create PeriodicReader
- Create SdkMeterProvider instance / or Pipelines with the above created reader.
- This should invoke worker, and collect cycle.
If not straightforward, we can park it for later. (Ideally, this test should have existed at first place before this PR.)
There was a problem hiding this comment.
I'll give it a shot 👍
|
Oops! Merge confict... Could you resolve that? |
|
Thanks @izquierdo for your contribution! Hope you can continue with more! We could really use more hands here. |
Add a success scenario test for PeriodicReader as suggested in #1481 (comment)
Fixes #1244
Changes
This defers starting the exporting thread until
MeterProviderBuilder#buildhas been called.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes