makes tracing easier#6
Conversation
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces tracing instrumentation across key execution paths (provider chat calls, JWT decoding, and agent message handling) to improve observability, and replaces a startup println! with a structured log event.
Changes:
- Add
#[tracing::instrument]spans tochat()implementations (OpenAI/Ollama), JWT decoding, and agent message handling. - Replace server startup
println!withtracing::info!. - Add
tracingas a dependency (and updateCargo.lockaccordingly).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/providers/openai.rs | Adds a tracing span around OpenAI::chat. |
| src/providers/ollama.rs | Adds a tracing span around OllamaAI::chat. |
| src/jwt.rs | Adds a tracing span around decode_jwt. |
| src/agent.rs | Uses tracing::info! for startup logging and instruments AgentWorker::message. |
| Cargo.toml | Adds tracing dependency. |
| Cargo.lock | Captures dependency graph update for tracing. |
Comments suppressed due to low confidence (1)
src/agent.rs:35
- Switching from
println!totracing::info!changes default behavior: without a globaltracingsubscriber, this message will be dropped and users won’t see the listening address anymore. If you want to keep the previous UX while still supporting tracing, consider either documenting/initializing a subscriber in the runnable example(s), or gating this log behind an optional feature (or falling back toprintln!).
tracing::info!("Listening on: {}", listener.local_addr());
listener
.map_err(|e| eprintln!("{}", e)) // TODO: Improve error handling.
.filter_map(|r| future::ready(r.ok()))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| println!("Listening on: {}", listener.local_addr()); | ||
| #[cfg(feature = "tracing")] | ||
| tracing::info!("Listening on: {}", listener.local_addr()); |
There was a problem hiding this comment.
AgentServer::run no longer prints the listening address when the tracing feature is disabled (previously println! ran unconditionally). This makes the default binary/library behavior less observable. Consider keeping a non-tracing fallback (e.g., println! under cfg(not(feature = "tracing"))) or emitting the message via a logging facade that works without requiring the tracing feature.
| tracing::info!("Listening on: {}", listener.local_addr()); | |
| tracing::info!("Listening on: {}", listener.local_addr()); | |
| #[cfg(not(feature = "tracing"))] | |
| println!("Listening on: {}", listener.local_addr()); |
| //! | ||
| //! ## Feature Flags | ||
| //! | ||
| //! **`tracing`**: Enables tracing for logging request/response traces. It requires a `RUST_LOG` environment variable to be set. |
There was a problem hiding this comment.
The docs claim the tracing feature "requires a RUST_LOG environment variable". This crate only adds tracing instrumentation and does not set up a subscriber/EnvFilter, so RUST_LOG is not inherently required and won’t have any effect unless the consumer configures a subscriber (e.g., tracing-subscriber with EnvFilter). Please reword to describe that users must install/initialize a tracing subscriber, and mention RUST_LOG only as an optional filter mechanism when using EnvFilter.
| //! **`tracing`**: Enables tracing for logging request/response traces. It requires a `RUST_LOG` environment variable to be set. | |
| //! **`tracing`**: Enables `tracing` instrumentation for request/response traces. | |
| //! To collect or display these traces, applications must install and initialize a | |
| //! tracing subscriber (for example, via `tracing-subscriber`). If the subscriber is | |
| //! configured with an `EnvFilter`, `RUST_LOG` may be used as an optional filtering | |
| //! mechanism. |
|
|
||
| ## Feature Flag | ||
|
|
||
| **`tracing`**: Enables tracing for logging request/response traces. It requires a `RUST_LOG` environment variable to be set. |
There was a problem hiding this comment.
The README states the tracing feature "requires a RUST_LOG environment variable". Since the crate does not configure a tracing subscriber, RUST_LOG is not required and won’t do anything unless the user sets up a subscriber with filtering (e.g., tracing-subscriber + EnvFilter). Please update this section to explain the need to initialize a subscriber and treat RUST_LOG as optional/config-dependent.
| **`tracing`**: Enables tracing for logging request/response traces. It requires a `RUST_LOG` environment variable to be set. | |
| **`tracing`**: Enables tracing instrumentation for request/response traces. To see tracing output, your application must initialize a tracing subscriber (for example, with `tracing-subscriber`). A `RUST_LOG` environment variable is not required by this crate itself; it is optional and only has an effect if your subscriber is configured to read environment-based filters (for example, via `tracing-subscriber::EnvFilter`). |
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
Fixes #3