feat(E9): add OTel metrics for CLI invocation rate/error#255
feat(E9): add OTel metrics for CLI invocation rate/error#255KooshaPari wants to merge 5 commits into
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: a780f51861
ℹ️ 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".
|
|
||
| use opentelemetry::{ | ||
| KeyValue, | ||
| metrics::{Counter, Histogram, Meter, UpDownCounter, Result}, |
There was a problem hiding this comment.
With byteport-otel pinned to opentelemetry = "0.28", this new crate is using pre-0.27 metric APIs: opentelemetry::metrics::Result was renamed and the instrument builders used throughout this file no longer expose .init() (they use .build()). As soon as the new workspace member is built, the crate fails to compile, so the CLI metrics feature cannot run until these calls are updated or the dependency is pinned to a compatible version.
Useful? React with 👍 / 👎.
| "crates/byteport-transport", | ||
| "crates/byteport-cli", | ||
| "crates/byteport-dag", | ||
| "crates/byteport-otel", |
There was a problem hiding this comment.
This adds crates/byteport-otel to the workspace, but the committed Cargo.lock still has no byteport-otel package, no OpenTelemetry dependencies, and even the byteport-cli entry lacks the newly added byteport-otel/tracing dependencies. Any cargo build --locked/frozen build of this commit will reject the stale lockfile, so please regenerate and commit the lockfile after adding the crate and dependencies.
Useful? React with 👍 / 👎.
| Err(e) => { | ||
| record_cli_error("codec", "encode_failure"); | ||
| eprintln!("Encode error: {e}"); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
On error paths such as an invalid codec operation, the code records the new error counter and then calls std::process::exit(1), which skips dropping _otel_guard in main. That means the just-recorded CLI error metrics and any pending spans are not flushed for exactly the failing invocations this feature is meant to count; return an exit status/result from main after the guard can shut down instead.
Useful? React with 👍 / 👎.
| TelemetryGuard { | ||
| _tracer_provider: tracer_provider, | ||
| } |
There was a problem hiding this comment.
The guard only retains the tracer provider, while the meter provider is moved into the global provider and no handle is kept for force_flush/shutdown. For short-lived CLI commands, the PeriodicReader is configured to export every 60 seconds, so successful invocations can increment byteport.cli.invocations and then exit before the meter reader ever exports them; keep the meter provider in TelemetryGuard and shut it down alongside traces.
Useful? React with 👍 / 👎.
| let _otel_guard = byteport_otel::init::init_default(); | ||
| info!("byteport-cli starting"); |
There was a problem hiding this comment.
init_default() enables the JSON fmt layer at info level, and these info! calls run for every normal CLI command, so commands like byteport-cli codec encode hello will emit JSON log records on stdout before/after the actual encoded value. That breaks scripts and tests that consume stdout as the command result; send logs to stderr or disable stdout logging by default for this CLI path.
Useful? React with 👍 / 👎.
|
Legacy Tooling Scan Report
No violations detected. This is a WARN-mode scan. Fix before strict enforcement begins. |
|
KooshaPari
added
area:compute-infra
1 participant
Summary