feat: Add OTLP export support for internal telemetry opentelemetry #1566
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1566 +/- ##
==========================================
+ Coverage 83.83% 83.87% +0.03%
==========================================
Files 433 440 +7
Lines 123668 123967 +299
==========================================
+ Hits 103678 103974 +296
- Misses 19456 19459 +3
Partials 534 534
🚀 New features to boost your workflow:
|
rust/otap-dataflow/crates/telemetry/src/opentelemetry_client.rs
Outdated
Show resolved
Hide resolved
rust/otap-dataflow/crates/telemetry/src/opentelemetry_client/meter_provider.rs
Show resolved
Hide resolved
| // If there is a tokio runtime already, use it. Otherwise, create a new one. | ||
| let tokio_runtime = match runtime { | ||
| Some(rt) => rt, | ||
| None => tokio::runtime::Runtime::new() |
There was a problem hiding this comment.
This part , I am not sure what is the right thing to do. If we just create a runtime like above, will it create new thread(s)? Or should we use the current-thread runtime? I am unsure how this would affect rest of df_engine.
There was a problem hiding this comment.
It basically creates the runtime "lazy/on-demand". It will be created only if needed, and only once.
There was a problem hiding this comment.
I am referring to which thread(s) the runtime will run on? The df_engine has some logic to ensure it runs on the threads (and cpu cores) requested by user. This is probably not an issue, but I don't have enough understand of the engine/controller to confirm.
There was a problem hiding this comment.
This Tokio runtime is created once in the main/controller thread, before the per-core pipeline threads are spawned. There are per-core runtimes, which are different.
There was a problem hiding this comment.
I also opened #1573 to discuss this more. Maybe there is no action required, but something to track.
rust/otap-dataflow/crates/telemetry/src/opentelemetry_client/meter_provider.rs
Outdated
Show resolved
Hide resolved
| OtlpProtocol::HttpJson => { | ||
| exporter = Self::configure_http_exporter(otlp_config, Protocol::HttpJson)? | ||
| } |
There was a problem hiding this comment.
Can HTTP/JSON be an optional feature, or even left out in this case?
There was a problem hiding this comment.
The user is not forced to configure it... only used if configured. It is not a separate library.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM.
Left some non-blocking comments about whether we need to specially handle the tokio runtime to align with rest of df_engine. It's worth discussing separately.
|
|
||
| /// The temporality of the metrics to be exported. | ||
| #[serde(default)] | ||
| pub temporality: Temporality, |
There was a problem hiding this comment.
For future iteration - good to add header support - for auth headers (bearer token, API-KEYs etc). otel-otlp supports adding headers in builder, so should be feasible to extend.
|
Changes looks good. Thanks :) |
4 participants
Add OTLP export support for internal telemetry opentelemetry integration.