feat(otlp): Allow to provide http client wrapped in Arc for user#3468
feat(otlp): Allow to provide http client wrapped in Arc for user#3468DoumanAsh wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3468 +/- ##
=====================================
Coverage 82.8% 82.9%
=====================================
Files 130 130
Lines 27262 27277 +15
=====================================
+ Hits 22595 22614 +19
+ Misses 4667 4663 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
137b17c to
05851cc
Compare
|
Hey @DoumanAsh thanks for raising this; it is a nice improvement to be able to re-use a HTTP client. Could you:
This is technically a breaking change, but I had a hunt for other impls of I note that zipkin has a similar-shaped |
| @@ -771,7 +771,10 @@ impl HasHttpConfig for HttpExporterBuilder { | |||
| /// ``` | |||
| pub trait WithHttpConfig { | |||
| /// Assign client implementation | |||
There was a problem hiding this comment.
I think it would be good to clarify that this can now take an Arc (or not), allowing the user to re-use HTTP clients if they so desire
There was a problem hiding this comment.
I updated documentation adding doc test that illustrates both usages (which also serves as way to verify it is not breaking change)
There was a problem hiding this comment.
Note that since I didn't want to break interface, it would require Arc with concrete HttpClient
Having Arc<dyn HttpClient> would be better, but it would not be compatible with just passing HttpClient instance
05851cc to
c903548
Compare
|
It seems codecov doesn't count coverage from doc tests, so I guess I can create just unit test for it |
scottgerring
left a comment
There was a problem hiding this comment.
Hey @DoumanAsh thanks for the updates! Couple suggestions inline - also could you please update opentelemetry-otlp/CHANGELOG.md and opentelemetry-zipkin/CHANGELOG.md with a vNext entry for each change?
6d6b586 to
7b03326
Compare
|
@scottgerring Sure, updated and added changelog changes |
7b03326 to
a6dcf21
Compare
|
@scottgerring I applied your comments and rebased my PR, please take a look |
Fixes #3467
Changes
This allows user to provide Arc wrapped client directly.
I initially thought it would be better to allow HttpClient to be Clone-able, but seeing as internally it is wrapped into Arc, I decided to allow user to do it himself so that it is possible for user to re-use this client when setting up multiple exporters
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes