refactor: Holds histogram in the timer to avoid clone labels if possible (#1653)

* feat: use Histogram struct to impl Timer

* fix: fix compile errors

* feat: downgrade metrics-process

* fix: compiler errors
This commit is contained in:
Yingwen
2023-05-26 17:12:03 +08:00
committed by GitHub
parent c042723fc9
commit 89366ba939
14 changed files with 121 additions and 147 deletions

87
Cargo.lock generated
View File

@@ -1222,7 +1222,7 @@ dependencies = [
"lazy_static",
"log-store",
"meta-client",
"metrics 0.21.0",
"metrics",
"mito",
"moka 0.11.0",
"object-store",
@@ -1860,7 +1860,7 @@ dependencies = [
"async-trait",
"common-error",
"common-telemetry",
"metrics 0.21.0",
"metrics",
"once_cell",
"paste",
"snafu",
@@ -1876,7 +1876,7 @@ dependencies = [
"backtrace",
"common-error",
"console-subscriber",
"metrics 0.21.0",
"metrics",
"metrics-exporter-prometheus",
"metrics-util",
"once_cell",
@@ -2577,7 +2577,7 @@ dependencies = [
"log-store",
"meta-client",
"meta-srv",
"metrics 0.21.0",
"metrics",
"mito",
"object-store",
"pin-project",
@@ -3149,7 +3149,7 @@ dependencies = [
"meta-srv",
"meter-core",
"meter-macros",
"metrics 0.21.0",
"metrics",
"mito",
"moka 0.9.7",
"object-store",
@@ -4263,7 +4263,7 @@ checksum = "cef509aa9bc73864d6756f0d34d35504af3cf0844373afe9b8669a5b8005a729"
dependencies = [
"console",
"number_prefix",
"portable-atomic 0.3.19",
"portable-atomic",
"unicode-width",
]
@@ -4988,7 +4988,7 @@ dependencies = [
"h2",
"http-body",
"lazy_static",
"metrics 0.21.0",
"metrics",
"once_cell",
"parking_lot",
"prost",
@@ -5034,32 +5034,22 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7b9b8653cec6897f73b519a43fba5ee3d50f62fe9af80b428accdcc093b4a849"
dependencies = [
"ahash 0.7.6",
"metrics-macros 0.6.0",
"portable-atomic 0.3.19",
]
[[package]]
name = "metrics"
version = "0.21.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "aa8ebbd1a9e57bbab77b9facae7f5136aea44c356943bf9a198f647da64285d6"
dependencies = [
"ahash 0.8.3",
"metrics-macros 0.7.0",
"portable-atomic 1.3.2",
"metrics-macros",
"portable-atomic",
]
[[package]]
name = "metrics-exporter-prometheus"
version = "0.12.1"
version = "0.11.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8a4964177ddfdab1e3a2b37aec7cf320e14169abb0ed73999f558136409178d5"
checksum = "8603921e1f54ef386189335f288441af761e0fc61bcb552168d9cedfe63ebc70"
dependencies = [
"base64 0.21.0",
"indexmap",
"metrics 0.21.0",
"metrics",
"metrics-util",
"quanta 0.11.0",
"parking_lot",
"portable-atomic",
"quanta 0.10.1",
"thiserror",
]
@@ -5074,26 +5064,15 @@ dependencies = [
"syn 1.0.109",
]
[[package]]
name = "metrics-macros"
version = "0.7.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ddece26afd34c31585c74a4db0630c376df271c285d682d1e55012197830b6df"
dependencies = [
"proc-macro2",
"quote",
"syn 2.0.15",
]
[[package]]
name = "metrics-process"
version = "1.0.10"
version = "1.0.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "767e7caea6cb64b66f96f7ab0e5f8bd8334d72dbbc522c99bed71d8a382d515f"
checksum = "99eab79be9f7c18565e889d6eaed6f1ebdafb2b6a88aef446d2fee5e7796ed10"
dependencies = [
"libproc",
"mach2",
"metrics 0.21.0",
"metrics",
"once_cell",
"procfs",
"rlimit",
@@ -5102,19 +5081,21 @@ dependencies = [
[[package]]
name = "metrics-util"
version = "0.15.0"
version = "0.14.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "111cb375987443c3de8d503580b536f77dc8416d32db62d9456db5d93bd7ac47"
checksum = "f7d24dc2dbae22bff6f1f9326ffce828c9f07ef9cc1e8002e5279f845432a30a"
dependencies = [
"aho-corasick 0.7.20",
"crossbeam-epoch",
"crossbeam-utils",
"hashbrown 0.13.2",
"hashbrown 0.12.3",
"indexmap",
"metrics 0.21.0",
"metrics",
"num_cpus",
"ordered-float 3.7.0",
"quanta 0.11.0",
"ordered-float 2.10.0",
"parking_lot",
"portable-atomic",
"quanta 0.10.1",
"radix_trie",
"sketches-ddsketch",
]
@@ -5634,7 +5615,7 @@ dependencies = [
"futures",
"lru 0.9.0",
"md5",
"metrics 0.21.0",
"metrics",
"opendal",
"pin-project",
"tokio",
@@ -5692,7 +5673,7 @@ dependencies = [
"hyper",
"log",
"md-5",
"metrics 0.20.1",
"metrics",
"once_cell",
"parking_lot",
"percent-encoding",
@@ -6298,12 +6279,6 @@ version = "0.3.19"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "26f6a7b87c2e435a3241addceeeff740ff8b7e76b74c13bf9acb17fa454ea00b"
[[package]]
name = "portable-atomic"
version = "1.3.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "dc59d1bcc64fc5d021d67521f818db868368028108d37f0e98d74e33f68297b5"
[[package]]
name = "postgres-protocol"
version = "0.6.5"
@@ -6770,7 +6745,7 @@ dependencies = [
"futures",
"futures-util",
"humantime",
"metrics 0.21.0",
"metrics",
"num",
"num-traits",
"object-store",
@@ -8186,7 +8161,7 @@ dependencies = [
"humantime-serde",
"hyper",
"influxdb_line_protocol",
"metrics 0.21.0",
"metrics",
"metrics-process",
"mime_guess",
"mysql_async",
@@ -8646,7 +8621,7 @@ dependencies = [
"futures-util",
"lazy_static",
"log-store",
"metrics 0.21.0",
"metrics",
"object-store",
"parquet",
"paste",

View File

@@ -84,7 +84,7 @@ tokio = { version = "1.28", features = ["full"] }
tokio-util = { version = "0.7", features = ["io-util", "compat"] }
tonic = { version = "0.9", features = ["tls"] }
uuid = { version = "1", features = ["serde", "v4", "fast-rng"] }
metrics = "0.21"
metrics = "0.20"
meter-core = { git = "https://github.com/GreptimeTeam/greptime-meter.git", rev = "f0798c4c648d89f51abe63e870919c75dd463199" }
[workspace.dependencies.meter-macros]

View File

@@ -12,8 +12,8 @@ deadlock_detection = ["parking_lot"]
backtrace = "0.3"
common-error = { path = "../error" }
console-subscriber = { version = "0.1", optional = true }
metrics-exporter-prometheus = { version = "0.12", default-features = false }
metrics-util = "0.15"
metrics-exporter-prometheus = { version = "0.11", default-features = false }
metrics-util = "0.14"
metrics.workspace = true
once_cell = "1.10"
opentelemetry = { version = "0.17", default-features = false, features = [

View File

@@ -17,7 +17,7 @@
use std::sync::{Arc, Once, RwLock};
use std::time::{Duration, Instant};
use metrics::histogram;
use metrics::{register_histogram, Histogram, IntoLabels};
use metrics_exporter_prometheus::PrometheusBuilder;
pub use metrics_exporter_prometheus::PrometheusHandle;
use metrics_util::layers::{Layer, PrefixLayer};
@@ -58,38 +58,45 @@ pub fn try_handle() -> Option<PrometheusHandle> {
PROMETHEUS_HANDLE.as_ref().read().unwrap().clone()
}
/// A Histogram timer that emits the elapsed time to the histogram on drop.
#[must_use = "Timer should be kept in a variable otherwise it cannot observe duration"]
#[derive(Debug)]
pub struct Timer {
start: Instant,
name: &'static str,
labels: Vec<(String, String)>,
histogram: Histogram,
}
impl From<Histogram> for Timer {
fn from(histogram: Histogram) -> Timer {
Timer::from_histogram(histogram)
}
}
impl Timer {
/// Creates a timer from given histogram.
pub fn from_histogram(histogram: Histogram) -> Self {
Self {
start: Instant::now(),
histogram,
}
}
/// Creates a timer from given `name`.
pub fn new(name: &'static str) -> Self {
Self {
start: Instant::now(),
name,
labels: Vec::new(),
histogram: register_histogram!(name),
}
}
pub fn new_with_labels<S: Into<String> + Clone>(name: &'static str, labels: &[(S, S)]) -> Self {
/// Creates a timer from given `name`.
pub fn new_with_labels<L: IntoLabels>(name: &'static str, labels: L) -> Self {
Self {
start: Instant::now(),
name,
labels: labels
.iter()
.map(|(k, v)| (k.clone().into(), v.clone().into()))
.collect::<Vec<_>>(),
histogram: register_histogram!(name, labels),
}
}
pub fn labels_mut(&mut self) -> &mut Vec<(String, String)> {
self.labels.as_mut()
}
/// Returns the elapsed duration from the time this timer created.
pub fn elapsed(&self) -> Duration {
self.start.elapsed()
}
@@ -97,11 +104,7 @@ impl Timer {
impl Drop for Timer {
fn drop(&mut self) {
if !self.labels.is_empty() {
histogram!(self.name, self.start.elapsed(), &self.labels);
} else {
histogram!(self.name, self.start.elapsed());
}
self.histogram.record(self.elapsed())
}
}
@@ -110,7 +113,7 @@ macro_rules! timer {
($name: expr) => {
$crate::metric::Timer::new($name)
};
($name:expr,$labels:expr) => {
($name:expr, $labels:expr) => {
$crate::metric::Timer::new_with_labels($name, $labels)
};
}
@@ -123,8 +126,7 @@ mod tests {
fn test_elapsed_timer() {
init_default_metrics_recorder();
{
let t = Timer::new("test_elapsed_timer_a");
drop(t);
let _t = timer!("test_elapsed_timer_a");
}
let handle = try_handle().unwrap();
let text = handle.render();
@@ -141,8 +143,7 @@ mod tests {
fn test_elapsed_timer_with_label() {
init_default_metrics_recorder();
{
let t = Timer::new("test_elapsed_timer_a");
drop(t);
let _t = timer!("test_elapsed_timer_a");
}
let handle = try_handle().unwrap();
let text = handle.render();
@@ -150,34 +151,16 @@ mod tests {
assert!(!text.contains("test_elapsed_timer_b"));
let label_a = "label_a";
let label_b = "label_b";
let label_c = "label_c";
let label_d = "label_d";
let label_e = "label_e";
assert!(!text.contains(label_a));
assert!(!text.contains(label_b));
{
let mut timer_b = timer!("test_elapsed_timer_b", &[(label_a, "a"), (label_b, "b")]);
let labels = timer_b.labels_mut();
labels.push((label_c.to_owned(), "d".to_owned()));
let _t = timer!("test_elapsed_timer_b", &[(label_a, "a"), (label_b, "b")]);
}
let text = handle.render();
assert!(text.contains("test_elapsed_timer_a"));
assert!(text.contains("test_elapsed_timer_b"));
assert!(text.contains(label_a));
assert!(text.contains(label_b));
assert!(text.contains(label_c));
{
let mut timer_c = timer!("test_elapsed_timer_c");
let labels = timer_c.labels_mut();
labels.push((label_d.to_owned(), "d".to_owned()));
labels.push((label_e.to_owned(), "e".to_owned()));
}
let text = handle.render();
assert!(text.contains("test_elapsed_timer_c"));
assert!(text.contains(label_d));
assert!(text.contains(label_e));
}
}

View File

@@ -47,7 +47,10 @@ impl router_server::Router for MetaSrv {
let _timer = timer!(
METRIC_META_ROUTE_REQUEST,
&[("op", "create"), ("cluster_id", &cluster_id.to_string())]
&[
("op", "create".to_string()),
("cluster_id", cluster_id.to_string())
]
);
let table_name = table_name.clone().context(error::EmptyTableNameSnafu)?;
@@ -73,7 +76,10 @@ impl router_server::Router for MetaSrv {
let _timer = timer!(
METRIC_META_ROUTE_REQUEST,
&[("op", "route"), ("cluster_id", &cluster_id.to_string())]
&[
("op", "route".to_string()),
("cluster_id", cluster_id.to_string())
]
);
let ctx = self.new_ctx();
@@ -88,7 +94,10 @@ impl router_server::Router for MetaSrv {
let _timer = timer!(
METRIC_META_ROUTE_REQUEST,
&[("op", "delete"), ("cluster_id", &cluster_id.to_string())]
&[
("op", "delete".to_string()),
("cluster_id", cluster_id.to_string())
]
);
let ctx = self.new_ctx();

View File

@@ -65,9 +65,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "range"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "range".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -103,9 +103,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "put"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "put".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -132,9 +132,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "batch_get"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "batch_get".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -175,9 +175,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "batch_put"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "batch_put".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -220,9 +220,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "batch_delete"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "batch_delete".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -269,9 +269,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "compare_and_put"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "compare_and_put".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -329,9 +329,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "delete_range"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "delete_range".to_string()),
("cluster_id", cluster_id.to_string())
]
);
@@ -367,9 +367,9 @@ impl KvStore for EtcdStore {
let _timer = timer!(
METRIC_META_KV_REQUEST,
&[
("target", "etcd"),
("op", "move_value"),
("cluster_id", &cluster_id.to_string())
("target", "etcd".to_string()),
("op", "move_value".to_string()),
("cluster_id", cluster_id.to_string())
]
);

View File

@@ -40,7 +40,8 @@ humantime-serde = "1.1"
hyper = { version = "0.14", features = ["full"] }
influxdb_line_protocol = { git = "https://github.com/evenyag/influxdb_iox", branch = "feat/line-protocol" }
metrics.workspace = true
metrics-process = "1"
# metrics-process 1.0.10 depends on metrics-0.21 but opendal depends on metrics-0.20.1
metrics-process = "<1.0.10"
mime_guess = "2.0"
num_cpus = "1.13"
once_cell = "1.16"

View File

@@ -62,7 +62,7 @@ impl GreptimeRequestHandler {
let _timer = timer!(
crate::metrics::METRIC_SERVER_GRPC_DB_REQUEST_TIMER,
&[(crate::metrics::METRIC_DB_LABEL, &query_ctx.get_db_string())]
&[(crate::metrics::METRIC_DB_LABEL, query_ctx.get_db_string())]
);
let handler = self.handler.clone();

View File

@@ -72,7 +72,7 @@ impl PrometheusGateway for PrometheusGatewayService {
crate::metrics::METRIC_SERVER_GRPC_PROM_REQUEST_TIMER,
&[(
crate::metrics::METRIC_DB_LABEL,
&query_context.get_db_string()
query_context.get_db_string()
)]
);
let result = self.handler.do_query(&prom_query, query_context).await;

View File

@@ -51,7 +51,10 @@ pub async fn sql(
let db = query_params.db.or(form_params.db);
let _timer = timer!(
crate::metrics::METRIC_HTTP_SQL_ELAPSED,
&[(crate::metrics::METRIC_DB_LABEL, db.as_deref().unwrap_or(""))]
&[(
crate::metrics::METRIC_DB_LABEL,
db.clone().unwrap_or_default()
)]
);
let resp = if let Some(sql) = &sql {
@@ -104,7 +107,10 @@ pub async fn promql(
let db = params.db.clone();
let _timer = timer!(
crate::metrics::METRIC_HTTP_PROMQL_ELAPSED,
&[(crate::metrics::METRIC_DB_LABEL, db.as_deref().unwrap_or(""))]
&[(
crate::metrics::METRIC_DB_LABEL,
db.clone().unwrap_or_default()
)]
);
let prom_query = params.into();

View File

@@ -51,7 +51,7 @@ pub async fn influxdb_write(
.unwrap_or_else(|| DEFAULT_SCHEMA_NAME.to_string());
let _timer = timer!(
crate::metrics::METRIC_HTTP_INFLUXDB_WRITE_ELAPSED,
&[(crate::metrics::METRIC_DB_LABEL, &db)]
&[(crate::metrics::METRIC_DB_LABEL, db.clone())]
);
let (catalog, schema) = parse_catalog_and_schema_from_client_database_name(&db);
let ctx = Arc::new(QueryContext::with(catalog, schema));

View File

@@ -57,7 +57,7 @@ pub async fn remote_write(
crate::metrics::METRIC_HTTP_PROMETHEUS_WRITE_ELAPSED,
&[(
crate::metrics::METRIC_DB_LABEL,
params.db.as_deref().unwrap_or("")
params.db.clone().unwrap_or_default()
)]
);
let ctx = if let Some(db) = params.db {
@@ -97,7 +97,7 @@ pub async fn remote_read(
crate::metrics::METRIC_HTTP_PROMETHEUS_READ_ELAPSED,
&[(
crate::metrics::METRIC_DB_LABEL,
params.db.as_deref().unwrap_or("")
params.db.clone().unwrap_or_default()
)]
);
let ctx = if let Some(db) = params.db {

View File

@@ -206,11 +206,11 @@ impl<W: AsyncWrite + Send + Sync + Unpin> AsyncMysqlShim<W> for MysqlInstanceShi
&[
(
crate::metrics::METRIC_MYSQL_SUBPROTOCOL_LABEL,
crate::metrics::METRIC_MYSQL_BINQUERY
crate::metrics::METRIC_MYSQL_BINQUERY.to_string()
),
(
crate::metrics::METRIC_DB_LABEL,
&self.session.context().get_db_string()
self.session.context().get_db_string()
)
]
);
@@ -254,11 +254,11 @@ impl<W: AsyncWrite + Send + Sync + Unpin> AsyncMysqlShim<W> for MysqlInstanceShi
&[
(
crate::metrics::METRIC_MYSQL_SUBPROTOCOL_LABEL,
crate::metrics::METRIC_MYSQL_TEXTQUERY
crate::metrics::METRIC_MYSQL_TEXTQUERY.to_string()
),
(
crate::metrics::METRIC_DB_LABEL,
&self.session.context().get_db_string()
self.session.context().get_db_string()
)
]
);

View File

@@ -51,11 +51,11 @@ impl SimpleQueryHandler for PostgresServerHandler {
&[
(
crate::metrics::METRIC_POSTGRES_SUBPROTOCOL_LABEL,
crate::metrics::METRIC_POSTGRES_SIMPLE_QUERY
crate::metrics::METRIC_POSTGRES_SIMPLE_QUERY.to_string()
),
(
crate::metrics::METRIC_DB_LABEL,
&self.query_ctx.get_db_string()
self.query_ctx.get_db_string()
)
]
);
@@ -357,11 +357,11 @@ impl ExtendedQueryHandler for PostgresServerHandler {
&[
(
crate::metrics::METRIC_POSTGRES_SUBPROTOCOL_LABEL,
crate::metrics::METRIC_POSTGRES_EXTENDED_QUERY
crate::metrics::METRIC_POSTGRES_EXTENDED_QUERY.to_string()
),
(
crate::metrics::METRIC_DB_LABEL,
&self.query_ctx.get_db_string()
self.query_ctx.get_db_string()
)
]
);