Conversation
Greptile SummaryThis PR introduces a
Confidence Score: 4/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class Meter {
-otel: opentelemetry::metrics::Meter
+query_scope(query_id: QueryID, name) Meter
+test_scope(name: &str) Meter
+u64_counter(name) Counter
+u64_counter_with_desc_and_unit(name, desc, unit) Counter
+f64_gauge(name) Gauge
+i64_up_down_counter(name) UpDownCounter
+duration_us_metric() Counter
+rows_in_metric() Counter
+rows_out_metric() Counter
}
class Counter {
-value: AtomicU64
-otel: opentelemetry::metrics::Counter~u64~
+add(value, key_values) u64
+load(ordering) u64
}
class Gauge {
-value: AtomicF64
-otel: opentelemetry::metrics::Gauge~f64~
+update(value, key_values)
+load(ordering) f64
}
class UpDownCounter {
-value: AtomicI64
-otel: opentelemetry::metrics::UpDownCounter~i64~
+add(value, key_values)
+load(ordering) i64
}
class DefaultRuntimeStats {
-duration_us: Counter
-rows_in: Counter
-rows_out: Counter
-node_kv: Vec~KeyValue~
+new(meter, node_info) DefaultRuntimeStats
+add_rows_in(rows)
+add_rows_out(rows)
+add_cpu_us(cpu_us)
}
class RuntimeNodeManager {
-active_tasks: UpDownCounter
-completed_tasks: Counter
-failed_tasks: Counter
-cancelled_tasks: Counter
+new(meter, runtime_stats, node_info) RuntimeNodeManager
+handle_task_event(event)
+export_snapshot() StatSnapshot
}
Meter --> Counter : creates
Meter --> Gauge : creates
Meter --> UpDownCounter : creates
DefaultRuntimeStats --> Counter : uses
RuntimeNodeManager --> Counter : uses
RuntimeNodeManager --> UpDownCounter : uses
Last reviewed commit: 776bcf3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn test_scope(name: &'static str) -> Self { | ||
| let otel = global::meter(name); | ||
| Self { otel } | ||
| } |
There was a problem hiding this comment.
test_scope is public and not #[cfg(test)]-gated
Meter::test_scope is intended only for test usage (as the name implies and as it's only used in #[cfg(test)] blocks), but it is declared pub fn without a #[cfg(test)] guard. This means the method is compiled into production binaries and exposed as part of the public API of the common_metrics crate.
Consider gating it with #[cfg(test)] or, if it also needs to be available to integration tests in other crates, #[cfg(any(test, feature = "test-utils"))]:
| pub fn test_scope(name: &'static str) -> Self { | |
| let otel = global::meter(name); | |
| Self { otel } | |
| } | |
| #[cfg(test)] | |
| pub fn test_scope(name: &'static str) -> Self { | |
| let otel = global::meter(name); | |
| Self { otel } | |
| } |
| pub fn u64_counter(&self, name: impl Into<Cow<'static, str>>) -> Counter { | ||
| Counter::new(&self.otel, name, None, None) | ||
| } | ||
|
|
||
| pub fn u64_counter_with_desc_and_unit( | ||
| &self, | ||
| name: impl Into<Cow<'static, str>>, | ||
| description: Option<Cow<'static, str>>, | ||
| unit: Option<Cow<'static, str>>, | ||
| ) -> Counter { | ||
| Counter::new(&self.otel, name, description, unit) | ||
| } |
There was a problem hiding this comment.
Two similar Counter-factory methods could be unified
u64_counter and u64_counter_with_desc_and_unit both create a Counter, differing only in whether description and unit are provided. Per the project's preference for single parametrised functions over multiple near-duplicate ones, these could be merged using Option parameters:
pub fn u64_counter(
&self,
name: impl Into<Cow<'static, str>>,
description: Option<Cow<'static, str>>,
unit: Option<Cow<'static, str>>,
) -> Counter {
Counter::new(&self.otel, name, description, unit)
}All call sites that currently pass no description/unit would use None, None, while sites needing metadata pass them directly — removing the need for the _with_desc_and_unit variant.
Rule Used: Prefer single parametrized functions over multiple... (source)
Learnt From
Eventual-Inc/Daft#5207
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Changes Made
Wrap the OTEL meter type to reduce duplicated code.