Add OTLP Exporting support matrix #2524
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
=====================================
Coverage 77.9% 77.9%
=====================================
Files 123 123
Lines 22944 22944
=====================================
Hits 17880 17880
Misses 5064 5064 ☔ View full report in Codecov by Sentry. |
|
|
||
| | **Exporter Type** | **Provider Creation** | **Logging** | **Network Clients** | **Status** | | ||
| |----------------------|---------------------------|------------------------|----------------------------|------------| | ||
| | **Simple** | Inside `tokio::Runtime` | Inside `tokio::Runtime` | `tonic` | Works | |
There was a problem hiding this comment.
nit: It would be nice to use check marks here similar to the test matrix for metrics: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/metrics/test.md
| | **Simple** | Inside `tokio::Runtime` | Inside `tokio::Runtime` | `tonic` | Works | | |
| | **Simple** | Inside `tokio::Runtime` | Inside `tokio::Runtime` | `tonic` | :white_check_mark: | |
|
Should we simplify this by grouping common scenarios. Eg, |
|
|
||
| ## Test Matrix | ||
|
|
||
| | **Exporter Type** | **Provider Creation** | **Logging** | **Network Clients** | **Status** | |
There was a problem hiding this comment.
I find it difficult to parse the table for "exhaustiveness". It might be easier if we break it down along some other axis into multiple tables.
Perhaps by network client - as then we'll get 4 tables of 8 rows?
| @@ -15,3 +15,42 @@ popped back out into the files output by the collector. | |||
| * Docker, for the test container | |||
| * TCP/4317 and TCP/4318 free on your local machine. If you are running another | |||
| collector, you'll need to stop it for the tests to run. | |||
|
|
|||
| ## Test Matrix | |||
|
|
|||
There was a problem hiding this comment.
Should we add:
| We aim to support and provide integration test coverage for every permutation of [A/B/C] |
... or something along these lines, so its clear what the desired state here is? It'd be helpful to identify gaps. I think optimally we'd link out to some architectural or design goal elsewhere, if we had one handy.
| | **Batch** | Outside `tokio::Runtime` | Outside `tokio::Runtime` | `reqwest` | Works | | ||
| | **Batch** | Outside `tokio::Runtime` | Outside `tokio::Runtime` | `reqwest-blocking` | Works | | ||
| | **Batch** | Outside `tokio::Runtime` | Outside `tokio::Runtime` | `hyper` | Works | | ||
|
|
There was a problem hiding this comment.
This is not really related to the PR at hand, but I also remain unconvinced that we are gaining much by supporting hyper/tonic/reqwest all at once; we started discussing this elsewhere (but I can't find the PR anymore). I struggle to find a strong need for anything more than a single client for async and sync calling, as it is 1/ an implementation detail that does not concern the caller and 2/ likely won't save space in the app anyway because of the way rust manages dep resolution.
There was a problem hiding this comment.
Similar to async part, Rust does not have a std library for making Http calls or asynchronous runtimes... So we need to provide some ability for users to bring-their-own, and provide a default implementation for at least 1 common one.
Draft.
To be filled in with works vs not-works.
Opening to get early feedback.