-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metric tap agent #161
Metric tap agent #161
Conversation
Pull Request Test Coverage Report for Build 9067937779Details
💛 - Coveralls |
tap-agent/src/metrics.rs
Outdated
@@ -34,7 +34,8 @@ async fn _run_server(port: u16) { | |||
.route("/metrics", get(handler_metrics)) | |||
.fallback(handler_404); | |||
let addr = SocketAddr::from(([0, 0, 0, 0], port)); | |||
let server = Server::bind(&addr).serve(app.into_make_service()); | |||
let listener = tokio::net::TcpListener::bind(addr).await.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use expect
instead of unwrap
and add information: "Failed to bind metrics address"
tap-agent/src/metrics.rs
Outdated
|
||
use axum::{http::StatusCode, response::IntoResponse, routing::get, Router}; | ||
use futures_util::FutureExt; | ||
use jsonrpsee::tracing::error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I can't remember why I used jsonrpsee::tracing::error
here https://github.com/semiotic-ai/timeline-aggregation-protocol/blob/c179dfedee1ed8078b358de3d35f4d051f74c9c1/tap_aggregator/src/metrics.rs#L8
But in any case, it should look less weird with tracing::error
instead.
|
||
lazy_static! { | ||
static ref RAVS_PER_SENDER_ALLOCATION: CounterVec = register_counter_vec!( | ||
format!("ravs_created_per_sender_allocation"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent with other labels. Sometimes you use sender_x_allocation
and sometimes sender_allocation
. Just pick either one and use it everywhere
lazy_static! { | ||
static ref CLOSED_ALLOCATIONS: Counter = register_counter!( | ||
format!("amount_of_closed_allocations"), | ||
"allocations closed", | ||
) | ||
.unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more specific as to what this is counting:
lazy_static! { | |
static ref CLOSED_ALLOCATIONS: Counter = register_counter!( | |
format!("amount_of_closed_allocations"), | |
"allocations closed", | |
) | |
.unwrap(); | |
} | |
lazy_static! { | |
static ref CLOSED_SENDER_ALLOCATIONS: Counter = register_counter!( | |
format!("closed_sender_allocation_total"), | |
"Total count of sender-allocation managers closed.", | |
) | |
.unwrap(); | |
} |
lazy_static! { | ||
static ref RAV_VALUE: GaugeVec = | ||
register_gauge_vec!(format!("rav_value"), "RAV Updated value", &["allocation"]).unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's missing the sender label.
lazy_static! { | |
static ref RAV_VALUE: GaugeVec = | |
register_gauge_vec!(format!("rav_value"), "RAV Updated value", &["allocation"]).unwrap(); | |
} | |
lazy_static! { | |
static ref RAV_VALUE: GaugeVec = | |
register_gauge_vec!(format!("rav_value"), "Value of the last RAV per sender-allocation", &["sender", "allocation"]).unwrap(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion about naming I thought of just now.
I think that we can use shorter names, for ex ravs_created_per_sender_allocation
can be ravs_created
, since we use the labels sender
and allocation
with it anyways.
fixes #156