fix(rust-sdk): fix Logger and make OpenTelemetry always-on#1382
Open
fix(rust-sdk): fix Logger and make OpenTelemetry always-on#1382
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant III as III_Runtime
participant Telemetry as OpenTelemetry
participant Handler
participant Exporter as OTLP_Exporter
Client->>III: invoke function (with incoming trace headers)
III->>Telemetry: extract trace context and baggage
III->>Handler: execute handler with telemetry context
Handler-->>III: return result
III->>Telemetry: start/end span, record logs, inject trace headers
Telemetry->>Exporter: export spans/logs/records
III->>Client: return response (with trace headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk/packages/rust/iii/README.md (1)
162-171:⚠️ Potential issue | 🟠 MajorREADME still advertises obsolete Rust registration APIs.
The new logging note is fine, but earlier snippets in this file still use
iii.register_function("id", handler)andiii.register_trigger("type", "fn", config). The current public API insdk/packages/rust/iii/src/iii.rsisRegisterFunction::new(...)/register_function_with(...)andregister_trigger(RegisterTriggerInput { ... }), so copy-paste users will hit compile errors.Current API shape
use iii_sdk::{RegisterFunction, RegisterTriggerInput}; iii.register_function(RegisterFunction::new("greet", |input: Value| async move { Ok(json!({ "message": "hello" })) })); iii.register_trigger(RegisterTriggerInput { trigger_type: "http".into(), function_id: "greet".into(), config: json!({ "api_path": "/greet", "http_method": "POST" }), })?;As per coding guidelines,
**/README.md: Check for related inconsistencies with the sdks/ and docs/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/README.md` around lines 162 - 171, Update README.md snippets that still call iii.register_function("id", handler) and iii.register_trigger("type", "fn", config) to use the current public API: construct RegisterFunction via RegisterFunction::new(...) or use register_function_with(...), and build triggers with RegisterTriggerInput { trigger_type, function_id, config, .. } passed to register_trigger(...); replace all outdated calls in the README examples with the corresponding RegisterFunction::new / register_function_with and RegisterTriggerInput usages (referencing the symbols RegisterFunction::new, register_function_with, RegisterTriggerInput, and register_trigger) so copy-paste examples compile.sdk/packages/rust/iii/src/iii.rs (1)
812-822:⚠️ Potential issue | 🟠 Major
shutdown_async()still doesn't give async callers a flush-safe shutdown path.The method only flips
runningand sendsOutbound::Shutdown, then returns immediately. With telemetry now auto-initialized inconnect(), async callers still have no way to wait fortelemetry::shutdown_otel().awaitbefore exit, so logs and spans can still be dropped.One possible fix
pub async fn shutdown_async(&self) { self.inner.running.store(false, Ordering::SeqCst); let _ = self.inner.outbound.send(Outbound::Shutdown); self.set_connection_state(IIIConnectionState::Disconnected); + if let Some(handle) = self.inner.connection_thread.lock_or_recover().take() { + let _ = tokio::task::spawn_blocking(move || { + let _ = handle.join(); + }) + .await; + } }Based on learnings,
shutdown()is deprecated in favor ofshutdown_async().awaitin the Rust SDK because that path is expected to guarantee telemetry flush before the process exits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/iii.rs` around lines 812 - 822, shutdown_async currently flips running and sends Outbound::Shutdown then returns immediately, which does not let async callers wait for run_connection() and telemetry::shutdown_otel() to complete; change shutdown_async to send the shutdown request then asynchronously wait for the connection thread to finish/acknowledge before returning. Concretely: after self.inner.outbound.send(Outbound::Shutdown) await a completion signal (e.g. a oneshot/Notify stored on self.inner like connection_done or shutdown_ack or await a JoinHandle/future for run_connection) so run_connection() can call telemetry::shutdown_otel().await and then signal completion; only set_connection_state(IIIConnectionState::Disconnected) after the await so callers get a flush-safe shutdown path (also update Inner to expose the notifier/JoinHandle if needed).
🧹 Nitpick comments (1)
sdk/packages/rust/iii/src/lib.rs (1)
56-57: Document thatotel: Nonenow means “use default telemetry settings.”
register_worker(..., InitOptions::default())now still initializes telemetry viaunwrap_or_default(), so these field docs are easy to read as an opt-out even though disabling now requiresenabled: Some(false).Suggested doc tweak
- /// OpenTelemetry configuration. + /// OpenTelemetry configuration. + /// `None` uses the SDK defaults and still initializes telemetry. + /// Set `enabled: Some(false)` to disable telemetry explicitly.As per coding guidelines,
sdk/**: Ensure the sdks are ergonomic and idiomatic within each language sdk while balancing being symmetric and consistent between language sdks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/packages/rust/iii/src/lib.rs` around lines 56 - 57, Update the doc comment for the struct field `otel: Option<crate::telemetry::types::OtelConfig>` to state that `None` means "use default telemetry settings" (so `register_worker(..., InitOptions::default())` will initialize telemetry via `unwrap_or_default()`), and clarify that to fully disable telemetry you must set `enabled: Some(false)` inside an explicit `OtelConfig`; reference `otel`, `InitOptions::default()`, `unwrap_or_default()`, and `crate::telemetry::types::OtelConfig` in the comment so readers know the semantics and how to opt out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sdk/packages/rust/iii/README.md`:
- Around line 162-171: Update README.md snippets that still call
iii.register_function("id", handler) and iii.register_trigger("type", "fn",
config) to use the current public API: construct RegisterFunction via
RegisterFunction::new(...) or use register_function_with(...), and build
triggers with RegisterTriggerInput { trigger_type, function_id, config, .. }
passed to register_trigger(...); replace all outdated calls in the README
examples with the corresponding RegisterFunction::new / register_function_with
and RegisterTriggerInput usages (referencing the symbols RegisterFunction::new,
register_function_with, RegisterTriggerInput, and register_trigger) so
copy-paste examples compile.
In `@sdk/packages/rust/iii/src/iii.rs`:
- Around line 812-822: shutdown_async currently flips running and sends
Outbound::Shutdown then returns immediately, which does not let async callers
wait for run_connection() and telemetry::shutdown_otel() to complete; change
shutdown_async to send the shutdown request then asynchronously wait for the
connection thread to finish/acknowledge before returning. Concretely: after
self.inner.outbound.send(Outbound::Shutdown) await a completion signal (e.g. a
oneshot/Notify stored on self.inner like connection_done or shutdown_ack or
await a JoinHandle/future for run_connection) so run_connection() can call
telemetry::shutdown_otel().await and then signal completion; only
set_connection_state(IIIConnectionState::Disconnected) after the await so
callers get a flush-safe shutdown path (also update Inner to expose the
notifier/JoinHandle if needed).
---
Nitpick comments:
In `@sdk/packages/rust/iii/src/lib.rs`:
- Around line 56-57: Update the doc comment for the struct field `otel:
Option<crate::telemetry::types::OtelConfig>` to state that `None` means "use
default telemetry settings" (so `register_worker(..., InitOptions::default())`
will initialize telemetry via `unwrap_or_default()`), and clarify that to fully
disable telemetry you must set `enabled: Some(false)` inside an explicit
`OtelConfig`; reference `otel`, `InitOptions::default()`, `unwrap_or_default()`,
and `crate::telemetry::types::OtelConfig` in the comment so readers know the
semantics and how to opt out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 116ac8aa-4ad7-4073-b2b2-61e83bde2860
📒 Files selected for processing (9)
console/packages/console-rust/Cargo.tomlengine/Cargo.tomlsdk/packages/rust/iii-example/Cargo.tomlsdk/packages/rust/iii/Cargo.tomlsdk/packages/rust/iii/README.mdsdk/packages/rust/iii/src/iii.rssdk/packages/rust/iii/src/lib.rssdk/packages/rust/iii/src/logger.rssdk/packages/rust/iii/tests/init_api.rs
💤 Files with no reviewable changes (2)
- sdk/packages/rust/iii/tests/init_api.rs
- sdk/packages/rust/iii/src/logger.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/packages/rust/iii-example/src/logger_example.rs`:
- Line 28: The description string ".description(\"Demonstrates Logger with all
log levels\")" is inaccurate because the example only exercises info/debug/warn;
update the example by either (A) adding an error-level log invocation (e.g.,
call log::error! or the example's logger.error-equivalent inside the same
function where info/debug/warn are called—look for the main function or the
block that emits those logs) so all levels are demonstrated, or (B) change the
description text to accurately reflect the exercised levels (e.g., "Demonstrates
Logger with info/debug/warn levels"); pick one and apply consistently to the
description string or logging calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4b777915-2270-4c8d-94d8-10c35a39e063
📒 Files selected for processing (2)
sdk/packages/rust/iii-example/src/logger_example.rssdk/packages/rust/iii-example/src/main.rs
✅ Files skipped from review due to trivial changes (1)
- sdk/packages/rust/iii-example/src/main.rs
| Ok::<Value, IIIError>(json!({ "status": "ok" })) | ||
| }, | ||
| ) | ||
| .description("Demonstrates Logger with all log levels"), |
Contributor
There was a problem hiding this comment.
Description says “all log levels”, but error is not exercised.
On Line 28, the description is broader than the implementation (Lines 11, 13, 18, 23 log only info/debug/warn). Please either add an error call or narrow the description text.
Suggested minimal text fix
- .description("Demonstrates Logger with all log levels"),
+ .description("Demonstrates Logger with multiple log levels"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .description("Demonstrates Logger with all log levels"), | |
| .description("Demonstrates Logger with multiple log levels"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/packages/rust/iii-example/src/logger_example.rs` at line 28, The
description string ".description(\"Demonstrates Logger with all log levels\")"
is inaccurate because the example only exercises info/debug/warn; update the
example by either (A) adding an error-level log invocation (e.g., call
log::error! or the example's logger.error-equivalent inside the same function
where info/debug/warn are called—look for the main function or the block that
emits those logs) so all levels are demonstrated, or (B) change the description
text to accurately reflect the exercised levels (e.g., "Demonstrates Logger with
info/debug/warn levels"); pick one and apply consistently to the description
string or logging calls.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
engine/src/modules/config.rs (1)
73-91: Consider caching the compiled regex.
expand_default_placeholderscompiles the same regex pattern on every call. While this function is likely called infrequently (once at startup), caching the regex in astatic OnceLock<Regex>would be more efficient and consistent with typical Rust patterns.♻️ Suggested optimization
+use std::sync::OnceLock; + +static DEFAULT_PLACEHOLDER_RE: OnceLock<Regex> = OnceLock::new(); + fn expand_default_placeholders(yaml_content: &str) -> String { - let re = Regex::new(r"\$\{([^}:]+)(?::([^}]*))?\}").unwrap(); + let re = DEFAULT_PLACEHOLDER_RE + .get_or_init(|| Regex::new(r"\$\{([^}:]+)(?::([^}]*))?\}").unwrap()); re.replace_all(yaml_content, |caps: ®ex::Captures| match caps.get(2) {Note: The same pattern applies to
expand_env_varsat line 46.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/modules/config.rs` around lines 73 - 91, The regex in expand_default_placeholders is compiled on every call; create a static OnceLock<Regex> (e.g. static PLACEHOLDER_RE: OnceLock<Regex>) and initialize it with the pattern using get_or_init, then replace the local Regex::new(...) in expand_default_placeholders to use PLACEHOLDER_RE.get().unwrap() (or get_or_init) to avoid recompilation; apply the same change to expand_env_vars (use a separate static or share if patterns match) and add the necessary use once_cell::sync::OnceLock import.engine/src/logging.rs (1)
316-318: Duplicate module lookup can be consolidated.
extract_otel_configalready iterates overcfg.modulesto findOtelModule. Lines 317-318 perform the same lookup again. Consider refactoring to extract both OtelConfig and log settings in a single pass, or haveextract_otel_configreturn additional fields.♻️ Suggested approach
Either extend
extract_otel_configto also returnlog_levelandlog_format:struct LoggingConfig { otel: OtelConfig, log_level: String, log_format: String, } fn extract_logging_config(cfg: &EngineConfig) -> LoggingConfig { // Single pass extraction }Or extract the module lookup into a shared helper to avoid duplicating the iteration logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/logging.rs` around lines 316 - 318, The code repeats the same cfg.modules iteration: extract_otel_config(cfg) already finds the OtelModule but lines creating otel_module_name and otel_module_cfg re-run that lookup; consolidate by either extending extract_otel_config (rename to extract_logging_config or return a struct containing OtelConfig plus log_level and log_format) or pull the module-finding logic into a shared helper (e.g., find_module_by_class) and use it from both places so only a single pass over cfg.modules is performed; update callers to use the new return type or helper and remove the duplicate iteration.engine/src/main.rs (1)
128-130: Consider inlining this trivial helper.The function
should_init_logging_from_engine_configsimply returnscli.use_default_config. While it adds a named abstraction, it may be over-engineering for a single boolean field access. Consider whether this indirection is necessary or if the condition could be inlined at the call site for clarity.That said, if the intent is to make the logging initialization decision more extensible in the future, the helper is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/main.rs` around lines 128 - 130, The trivial helper should be inlined: remove the function should_init_logging_from_engine_config and replace its call sites with direct access to the boolean field (cli.use_default_config) so the condition is evaluated inline where logging initialization is decided; if you prefer to keep the abstraction, at minimum add a comment explaining future extensibility and keep the function as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/logging.rs`:
- Around line 315-342: The println! call inside init_log_from_engine_config that
prints log_level, log_format, and otel_cfg.enabled should be removed or replaced
with a tracing::debug! call so it doesn't unconditionally print in production;
update the println! to tracing::debug! (or remove it) and ensure the message
uses the same variables (log_level, log_format, otel_cfg) and is emitted only
after tracing is initialized or where debug logs are appropriate.
---
Nitpick comments:
In `@engine/src/logging.rs`:
- Around line 316-318: The code repeats the same cfg.modules iteration:
extract_otel_config(cfg) already finds the OtelModule but lines creating
otel_module_name and otel_module_cfg re-run that lookup; consolidate by either
extending extract_otel_config (rename to extract_logging_config or return a
struct containing OtelConfig plus log_level and log_format) or pull the
module-finding logic into a shared helper (e.g., find_module_by_class) and use
it from both places so only a single pass over cfg.modules is performed; update
callers to use the new return type or helper and remove the duplicate iteration.
In `@engine/src/main.rs`:
- Around line 128-130: The trivial helper should be inlined: remove the function
should_init_logging_from_engine_config and replace its call sites with direct
access to the boolean field (cli.use_default_config) so the condition is
evaluated inline where logging initialization is decided; if you prefer to keep
the abstraction, at minimum add a comment explaining future extensibility and
keep the function as-is.
In `@engine/src/modules/config.rs`:
- Around line 73-91: The regex in expand_default_placeholders is compiled on
every call; create a static OnceLock<Regex> (e.g. static PLACEHOLDER_RE:
OnceLock<Regex>) and initialize it with the pattern using get_or_init, then
replace the local Regex::new(...) in expand_default_placeholders to use
PLACEHOLDER_RE.get().unwrap() (or get_or_init) to avoid recompilation; apply the
same change to expand_env_vars (use a separate static or share if patterns
match) and add the necessary use once_cell::sync::OnceLock import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b8f6f1e7-dfbd-4db4-b665-8674be080b71
📒 Files selected for processing (5)
docs/advanced/deployment.mdxengine/README.mdengine/src/logging.rsengine/src/main.rsengine/src/modules/config.rs
✅ Files skipped from review due to trivial changes (2)
- engine/README.md
- docs/advanced/deployment.mdx
Comment on lines
+315
to
+342
| pub fn init_log_from_engine_config(cfg: &EngineConfig) { | ||
| let otel_cfg = extract_otel_config(cfg); | ||
| let otel_module_name = "modules::observability::OtelModule"; | ||
| let otel_module_cfg = cfg.modules.iter().find(|m| m.class == otel_module_name); | ||
|
|
||
| let log_level = otel_module_cfg | ||
| .and_then(|m| m.config.as_ref()) | ||
| .and_then(|c| c.get("level").or_else(|| c.get("log_level"))) | ||
| .and_then(|v| v.as_str().map(|s| s.to_string())) | ||
| .unwrap_or_else(|| "info".to_string()); | ||
|
|
||
| let log_format = otel_module_cfg | ||
| .and_then(|m| m.config.as_ref()) | ||
| .and_then(|c| c.get("format")) | ||
| .and_then(|v| v.as_str().map(|s| s.to_string())) | ||
| .unwrap_or_else(|| "default".to_string()); | ||
|
|
||
| println!( | ||
| "Log level from config: {}, Log format: {}, OTel enabled: {}", | ||
| log_level, log_format, otel_cfg.enabled | ||
| ); | ||
|
|
||
| if log_format.to_lowercase() == "json" { | ||
| init_prod_log(log_level.as_str(), &otel_cfg); | ||
| } else { | ||
| init_local_log(log_level.as_str(), &otel_cfg); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Remove debug println! or convert to tracing::debug!.
Line 332-335 contains a println! statement that outputs configuration details. This will appear unconditionally in production output. Consider removing it or converting to tracing::debug! for consistency with the rest of the codebase.
🛠️ Suggested fix
- println!(
- "Log level from config: {}, Log format: {}, OTel enabled: {}",
- log_level, log_format, otel_cfg.enabled
- );
+ // If debugging info is needed, use tracing after initialization
+ // or remove this line entirely for cleaner startup outputAlternatively, if this info is valuable, log it after tracing is initialized or use a different mechanism.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn init_log_from_engine_config(cfg: &EngineConfig) { | |
| let otel_cfg = extract_otel_config(cfg); | |
| let otel_module_name = "modules::observability::OtelModule"; | |
| let otel_module_cfg = cfg.modules.iter().find(|m| m.class == otel_module_name); | |
| let log_level = otel_module_cfg | |
| .and_then(|m| m.config.as_ref()) | |
| .and_then(|c| c.get("level").or_else(|| c.get("log_level"))) | |
| .and_then(|v| v.as_str().map(|s| s.to_string())) | |
| .unwrap_or_else(|| "info".to_string()); | |
| let log_format = otel_module_cfg | |
| .and_then(|m| m.config.as_ref()) | |
| .and_then(|c| c.get("format")) | |
| .and_then(|v| v.as_str().map(|s| s.to_string())) | |
| .unwrap_or_else(|| "default".to_string()); | |
| println!( | |
| "Log level from config: {}, Log format: {}, OTel enabled: {}", | |
| log_level, log_format, otel_cfg.enabled | |
| ); | |
| if log_format.to_lowercase() == "json" { | |
| init_prod_log(log_level.as_str(), &otel_cfg); | |
| } else { | |
| init_local_log(log_level.as_str(), &otel_cfg); | |
| } | |
| } | |
| pub fn init_log_from_engine_config(cfg: &EngineConfig) { | |
| let otel_cfg = extract_otel_config(cfg); | |
| let otel_module_name = "modules::observability::OtelModule"; | |
| let otel_module_cfg = cfg.modules.iter().find(|m| m.class == otel_module_name); | |
| let log_level = otel_module_cfg | |
| .and_then(|m| m.config.as_ref()) | |
| .and_then(|c| c.get("level").or_else(|| c.get("log_level"))) | |
| .and_then(|v| v.as_str().map(|s| s.to_string())) | |
| .unwrap_or_else(|| "info".to_string()); | |
| let log_format = otel_module_cfg | |
| .and_then(|m| m.config.as_ref()) | |
| .and_then(|c| c.get("format")) | |
| .and_then(|v| v.as_str().map(|s| s.to_string())) | |
| .unwrap_or_else(|| "default".to_string()); | |
| // If debugging info is needed, use tracing after initialization | |
| // or remove this line entirely for cleaner startup output | |
| if log_format.to_lowercase() == "json" { | |
| init_prod_log(log_level.as_str(), &otel_cfg); | |
| } else { | |
| init_local_log(log_level.as_str(), &otel_cfg); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/logging.rs` around lines 315 - 342, The println! call inside
init_log_from_engine_config that prints log_level, log_format, and
otel_cfg.enabled should be removed or replaced with a tracing::debug! call so it
doesn't unconditionally print in production; update the println! to
tracing::debug! (or remove it) and ensure the message uses the same variables
(log_level, log_format, otel_cfg) and is emitted only after tracing is
initialized or where debug logs are appropriate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/modules/observability/otel.rs`:
- Around line 956-960: The current code always merges baggage via
BaggagePropagator::new().extract_with_context(&ctx, &carrier) even when a
present traceparent header failed to parse; change this so that after
TraceContextPropagator::new().extract(&carrier) you check whether the carrier
contained a traceparent header and the extracted trace context is invalid (use
the span/SpanContext validity API such as SpanContext::is_valid or
trace_id/SpanId validity on the extracted context), and if so DO NOT call
BaggagePropagator::new().extract_with_context (return the extracted ctx
unchanged) — only merge baggage when either there was no traceparent header or
the parsed trace context is valid; reference
TraceContextPropagator::new().extract,
BaggagePropagator::new().extract_with_context, and the carrier/traceparent
header in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66fecfaf-7d02-4cba-ad1d-c6fba1e0e0da
📒 Files selected for processing (1)
engine/src/modules/observability/otel.rs
Comment on lines
+956
to
+960
| // Extract trace context first, then merge baggage into that context. | ||
| // This avoids relying on global propagator state and keeps behavior | ||
| // aligned with the SDK helper when one header is invalid or absent. | ||
| let ctx = TraceContextPropagator::new().extract(&carrier); | ||
| BaggagePropagator::new().extract_with_context(&ctx, &carrier) |
Contributor
There was a problem hiding this comment.
Drop baggage when traceparent is present but invalid.
At Line 959 and Line 960, baggage is merged even when trace-context parsing fails. Through engine/src/telemetry.rs (Line 39 condition) this lets malformed/untrusted headers still set baggage values that downstream code can read (e.g., engine::baggage::get in engine/src/modules/observability/mod.rs Lines 386-399).
🔒 Suggested hardening
pub fn extract_context(traceparent: Option<&str>, baggage: Option<&str>) -> Context {
@@
- let ctx = TraceContextPropagator::new().extract(&carrier);
- BaggagePropagator::new().extract_with_context(&ctx, &carrier)
+ let ctx = TraceContextPropagator::new().extract(&carrier);
+
+ // If caller sent a traceparent but it is malformed/invalid, do not trust baggage.
+ if traceparent.is_some() && !ctx.span().span_context().is_valid() {
+ return ctx;
+ }
+
+ BaggagePropagator::new().extract_with_context(&ctx, &carrier)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/modules/observability/otel.rs` around lines 956 - 960, The current
code always merges baggage via
BaggagePropagator::new().extract_with_context(&ctx, &carrier) even when a
present traceparent header failed to parse; change this so that after
TraceContextPropagator::new().extract(&carrier) you check whether the carrier
contained a traceparent header and the extracted trace context is invalid (use
the span/SpanContext validity API such as SpanContext::is_valid or
trace_id/SpanId validity on the extracted context), and if so DO NOT call
BaggagePropagator::new().extract_with_context (return the extracted ctx
unchanged) — only merge baggage when either there was no traceparent header or
the parsed trace context is valid; reference
TraceContextPropagator::new().extract,
BaggagePropagator::new().extract_with_context, and the carrier/traceparent
header in your change.
guibeira
force-pushed
the
fix/rust-sdk-logger
branch
from
a9e14ad to
197a100
Compare
…nfig provided The Logger silently did nothing because init_otel() was only called when the user explicitly provided OtelConfig. The Python SDK always initializes OTel (with defaults when no config given). Match that behavior so Logger works out of the box.
Registers example::logger_demo function that exercises Logger with all log levels (info, debug, warn) and structured data. Invoked automatically during example run to verify logs reach the engine.
guibeira
force-pushed
the
fix/rust-sdk-logger
branch
from
a3bd93c to
b366279
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
otelfeature flag from the Rust SDK, making OpenTelemetry dependencies always included (no longer optional)Logger.info()etc. silently dropped all messages becauseinit_otel()was only called when the user explicitly providedOtelConfig. Now OTel auto-initializes with defaults (matching the Python SDK behavior)#[cfg(feature = "otel")]annotations (~30 occurrences across 4 source files + 1 test)features = ["otel"]from 3 downstreamCargo.tomlfiles (engine, iii-example, console-rust)Test plan
cargo checkpasses (0 errors)cargo test --no-runcompiles all test binariescfg(feature = "otel")references in source or testsInitOptions::default()(no explicitOtelConfigneeded)OtelConfigpassed viaInitOptionsSummary by CodeRabbit
New Features
Documentation
Tests
Engine