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

otlp: spawn thread to create blocking reqwest client #2431

Merged

Conversation

pitoniak32
Copy link
Contributor

fixes #2400

Changes

Using a new thread to create reqwest blocking client to avoid panics from async main

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@pitoniak32
Copy link
Contributor Author

I still need to test this, just getting this up to make sure its not off track.

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.4%. Comparing base (9cf7a40) to head (7b3c1c9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2431   +/-   ##
=====================================
  Coverage   79.4%   79.4%           
=====================================
  Files        122     122           
  Lines      21697   21700    +3     
=====================================
+ Hits       17240   17247    +7     
+ Misses      4457    4453    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lalitb
Copy link
Member

lalitb commented Dec 15, 2024

I still need to test this, just getting this up to make sure its not off track.

Thanks @pitoniak32. You can uncomment the integration test for reqwest-blocking to verify if it functions correctly with these changes? This mayn't necessarily work as there could be some issue with the integration test, I can check that separately in that case.

@cijothomas
Copy link
Member

@pitoniak32 @lalitb
I can confirm this is working as expected. https://github.com/open-telemetry/opentelemetry-rust/compare/main...cijothomas:cijothomas/testreq-block?expand=1 is the change I tried. Obviously, had to turn off logs/traces as they are using tokio, and won't work. But once that is also addressed, this should achieve what we want - which is to use background Thread + req::Blocking by default

@lalitb
Copy link
Member

lalitb commented Dec 16, 2024

@pitoniak32 @lalitb I can confirm this is working as expected. https://github.com/open-telemetry/opentelemetry-rust/compare/main...cijothomas:cijothomas/testreq-block?expand=1 is the change I tried. Obviously, had to turn off logs/traces as they are using tokio, and won't work. But once that is also addressed, this should achieve what we want - which is to use background Thread + req::Blocking by default

thanks @cijothomas. That makes me realize that the reqwest-blocking integration tests for logs and traces won't work till we support running batch exporter as separate dedicated thread. @pitoniak32 - please ignore my earlier comment on enabling and validating integration test. Sorry for confusion :)

@lalitb
Copy link
Member

lalitb commented Dec 17, 2024

I can confirm these changes also work for batch log exporter modeled as separate thread in #2436.

@pitoniak32 pitoniak32 marked this pull request as ready for review December 17, 2024 14:22
@pitoniak32 pitoniak32 requested a review from a team as a code owner December 17, 2024 14:22
Some(Arc::new(reqwest::blocking::Client::new()) as Arc<dyn HttpClient>)
})
.join()
.expect("creating reqwest::blocking::Client on a new thread not to fail");
Copy link
Member

Choose a reason for hiding this comment

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

maybe revisit this, to do an internal error log, and store None.?

@cijothomas cijothomas merged commit eb8d7c6 into open-telemetry:main Dec 17, 2024
21 checks passed
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.

OTLP Exporter - initialize Reqwest::Blocking client must be done in non async context
3 participants