feat(E7): add OTel tracing to CLI command lifecycle#254
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Legacy Tooling Scan Report
No violations detected. This is a WARN-mode scan. Fix before strict enforcement begins. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d7fd86c2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let _otel_guard = byteport_otel::init::init_default(); | ||
| info!("byteport-cli starting"); |
There was a problem hiding this comment.
With init_default() the CLI uses the default telemetry config where stdout JSON logging is enabled, and the newly added info! calls now write log records into the same stream as command results. For scriptable commands such as codec encode or upload, callers no longer receive just the encoded value or upload instruction on stdout. Initialize CLI telemetry with stdout logging disabled or route logs to stderr.
Useful? React with 👍 / 👎.
| Ok(mp) => { | ||
| opentelemetry::global::set_meter_provider(mp); |
There was a problem hiding this comment.
When metrics are enabled, the MeterProvider is installed globally and then discarded; TelemetryGuard only retains and shuts down the tracer provider, while the PeriodicReader exports every 60 seconds. Short-lived CLI commands record invocation/error metrics and exit before that interval, so the new CLI metrics can be lost at shutdown. Keep the meter provider in the guard and force-flush/shut it down on drop.
Useful? React with 👍 / 👎.
| let _span = start_cli_command_span("codec"); | ||
| run_codec(action); |
There was a problem hiding this comment.
Storing the OpenTelemetry span in _span does not make it the current context. The run_* helpers call start_cli_sub_span(), which reads Context::current(), so for every CLI command the execute/operation spans are created as roots or siblings instead of children of the command lifecycle span. Attach or enter the command span for the duration of the run_* call.
Useful? React with 👍 / 👎.
| "crates/byteport-transport", | ||
| "crates/byteport-cli", | ||
| "crates/byteport-dag", | ||
| "crates/byteport-otel", |
There was a problem hiding this comment.
AGENTS.md §7 marks Cargo.toml [workspace.members] as do-not-touch without an explicit L2 SOTA task; adding crates/byteport-otel here changes the workspace and CI surface for every cargo --workspace command despite that guardrail. Please split this under the required approval/task or avoid changing the root workspace membership.
Useful? React with 👍 / 👎.
|
Legacy Tooling Scan Report
No violations detected. This is a WARN-mode scan. Fix before strict enforcement begins. |
|
1 participant
Summary