feat(libsy): add OTel metrics, tracing spans, and structured logging#69
feat(libsy): add OTel metrics, tracing spans, and structured logging#69eric-liu-nvidia wants to merge 10 commits into
Conversation
22913d7 to
96d2593
Compare
Walkthroughlibsy adds a required algorithm name, instruments algorithm runs and LLM calls with tracing and OpenTelemetry metrics, records routing decisions, re-exports Changeslibsy observability
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/libsy/tests/observability.rs (1)
194-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared traversal logic between
u64_counter_valueandf64_histogram_count.Both functions repeat the same snapshot → scope-name-filter → metric-name-filter loop, differing only in the
AggregatedMetricsvariant and the value extracted. Worth factoring into one generic helper to avoid the two implementations drifting apart.♻️ Sketch of a shared helper
fn matching_data_points<'a, T>( snapshots: &'a [ResourceMetrics], name: &str, wanted: &[(&str, &str)], extract: impl Fn(&AggregatedMetrics) -> Option<&dyn Iterator<Item = &'a T>>, ) -> Option<u64> { // shared scope/metric-name filtering, delegate value extraction to callers // (or simply parameterize on an `impl Fn(&Metric) -> Option<u64>` per data point) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/libsy/tests/observability.rs` around lines 194 - 253, Extract the duplicated snapshot, scope-name, metric-name, and attribute filtering from u64_counter_value and f64_histogram_count into a shared helper. Parameterize the helper with the metric-data/value extraction needed for each AggregatedMetrics variant, then have both functions reuse it while preserving their existing cumulative maximum behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/libsy/README.md`:
- Around line 153-154: Update the impl Algorithm for LlmClassifierOrchAlgo
example to implement the required name method, returning the classifier’s stable
telemetry label, so the documented snippet compiles.
In `@crates/libsy/src/lib.rs`:
- Around line 298-300: Update the info method to await
self.driver.info(decision) before calling observability::record_decision, and
only record the decision after the driver returns success; propagate any driver
error without recording it.
---
Nitpick comments:
In `@crates/libsy/tests/observability.rs`:
- Around line 194-253: Extract the duplicated snapshot, scope-name, metric-name,
and attribute filtering from u64_counter_value and f64_histogram_count into a
shared helper. Parameterize the helper with the metric-data/value extraction
needed for each AggregatedMetrics variant, then have both functions reuse it
while preserving their existing cumulative maximum behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c15084e-1e09-4aa7-8c59-f212393feaa9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (9)
.agents/skills/switchyard-codebase-exploration/SKILL.mdcrates/libsy-examples/src/ensemble.rscrates/libsy-examples/src/llm_class.rscrates/libsy-examples/src/rand.rscrates/libsy/Cargo.tomlcrates/libsy/README.mdcrates/libsy/src/lib.rscrates/libsy/src/observability.rscrates/libsy/tests/observability.rs
| .fulfill_request::<RoutedRequest, Response>(routed) | ||
| .await | ||
| let selected_model = routed.decision.selected_model().to_string(); | ||
| let span = observability::llm_call_span(&self.algorithm, &selected_model); |
There was a problem hiding this comment.
We need to be careful with labeling here. This is the time, from the algorithms perspective on how long it took to fulfill the model request.
We should also have a span for the api call itself. ie in LlmClient.
There was a problem hiding this comment.
docs now state libsy.llm_call measures fulfillment as the algorithm observes it (host queueing/serving included), and a new libsy.client_call span wraps the actual provider call in the one place libsy performs it (run's default-client serve), with outcome/error recorded; hosts serving over their own transport are pointed at spanning their LlmClient equivalently.
| async { | ||
| let started = Instant::now(); | ||
| let result = self | ||
| .driver | ||
| .fulfill_request::<RoutedRequest, Response>(routed) | ||
| .await; | ||
| observability::record_llm_call( | ||
| &self.algorithm, | ||
| &selected_model, | ||
| started.elapsed(), | ||
| &result, | ||
| &tracing::Span::current(), | ||
| ); | ||
| result | ||
| } | ||
| .instrument(span) | ||
| .await |
There was a problem hiding this comment.
This seems ugly to me. Can we have a helper so there is not so much code just to add a span? It's hard to tell what is really going on which is just a simple driver.fulfill_request
Is there not some rusty way of instrumenting the method. Like using an raii object.
There was a problem hiding this comment.
new observe_llm_call / observe_run future-wrapping helpers own all span/timing/record plumbing; call_llm is now three lines that read as "fulfill_request, observed". (A literal RAII guard doesn't work here — tracing's entered-guards can't be held across .awaits — so the future adapter is the rusty idiom.)
There was a problem hiding this comment.
I still think this could be cleaner. Could we use something like https://crates.io/crates/tracing
and https://github.com/tokio-rs/tracing-opentelemetry
That way it could look something like
use tracing::{info, info_span};
fn process_data() {
let span = info_span!("process_data");
let _guard = span.enter(); // RAII guard acquired
info!("Doing work inside the OTel-backed span");
} // _guard is dropped here, automatically exiting the spanThere was a problem hiding this comment.
nvm I missed the await spans + async thing.
There was a problem hiding this comment.
Looks like we can use `#[instrument] in the methods though.
There was a problem hiding this comment.
span.enter() guards can't be held across .await — the tracing docs call this out directly: "Holding the drop guard returned by Span::enter across .await points will result in incorrect traces." (https://docs.rs/tracing/latest/tracing/struct.Span.html#in-asynchronous-code) For async code, the docs prescribe Future::instrument, which is exactly what observe_llm_call uses — we're already on tracing + the tracing-opentelemetry bridge. I tried the guard version to be sure: our own span test then shows client_call mis-parented under llm_call (thread-state leakage from the suspended task). 1e7dbde adds an assertion that fails red under the guard pattern.
Also, the call site is now three lines after the earlier hoist — this snippet is outdated.
There was a problem hiding this comment.
Good call — #[instrument] works for the method spans, and it came out nicer: call_llm and serve now declare their spans as attributes, and the three span-constructor helpers are gone (net −23 lines). Two caveats: the libsy.run span keeps explicit Future::instrument because it wraps the spawned run task rather than a method body, and the timing/metrics recording stays in-body since it needs the result and elapsed time. The parentage tests pass unchanged, so the traces are identical. Done in af6f871.
da2b662 to
27ab3f4
Compare
|
I think this PR tries to do too much. Should we start by defining what metrics libsy should report? That's probably a conversation for Slack. Then we could add one in a small PR (just the new dependency and the one metric line), and go from there. |
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
…e_llm_call Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
…ming Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
…kage Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
af6f871 to
3987417
Compare
3 participants
Summary
Implements SWITCH-928: the observability layer for the libsy algorithm layer, built on OpenTelemetry per the discussion on the ticket (opentelemetry-rust as the metrics sink;
tracingfor spans and structured logs, which hosts bridge into OTel withtracing-opentelemetry).All instrumentation lives in the libsy core at the
Decisionhook and the offload boundary, so algorithms carry no telemetry code beyond the newAlgorithm::name()label:libsy; the library owns no exporter): counterslibsy.runs,libsy.llm_calls,libsy.decisions,libsy.{input,output,total,reasoning}_tokens; histogramslibsy.run_duration_ms,libsy.llm_call_duration_ms. Attributes arealgorithm/selected_model/outcome(ok|error), so failure rates fall out of theoutcomesplit. Instrument naming and cardinality bounds follow the switchyard OTel conventions.tracing): onelibsy.runper request carrying theMetadatacorrelation ids and outcome, with one childlibsy.llm_callper offloaded call carrying the selected model, latency, outcome, and token counts.tracing, targetlibsy): an info event per publishedDecisionwith its reasoning; warn events for failed calls and runs.API changes: new required
Algorithm::name(&self) -> &str(implemented for the three reference algorithms),Usagere-exported fromlibsy, and the unusedDriver::default()removed (drivers now carry the algorithm label).With no OTel SDK or tracing subscriber installed, all instrumentation is a no-op.
Testing
crates/libsy/tests/observability.rsassert the success path (counter/histogram values, exact token counts, span parentage and fields, decision log) and the failure path (outcome=errorat run and call level, error text on spans, warn logs) via an in-memory OTel exporter and a capturingtracinglayer.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspaceall pass (50/50 test binaries).Fixes SWITCH-928
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests