feat: add Tokenserver metrics #1200
Conversation
ethowitz
force-pushed
the
feat/add-tokenserver-logging-metrics
branch
from
8bb596d to
9c4907d
Compare
| @@ -211,6 +214,9 @@ impl TokenserverDb { | |||
| "#; | |||
| const DEFAULT_CAPACITY_RELEASE_RATE: f32 = 0.1; | |||
|
|
|||
| let mut metrics = self.metrics.clone(); | |||
| metrics.start_timer("tokenserver.storage.get_best_node", None); | |||
There was a problem hiding this comment.
You may have intended this but I just wanted to note that Cadence hardcodes a prefix that we set to the value of Settings::statsd_label so this emits "syncstorage.tokenserver.storage.get_best_node".
We could overwrite tokenserver's statsd_label to use "tokenserver" instead or look to the future with "syncstorage.tokenserver" or maybe it makes more sense to keep it as you have it 🤷♂️
There was a problem hiding this comment.
Ooo I didn't notice this. I think it makes sense to overwrite statsd_label to "syncstorage.tokenserver" for Tokenserver. I'll make the change
| @@ -241,7 +245,11 @@ impl Server { | |||
| let tokenserver_state = if settings.tokenserver.enabled { | |||
| Some(tokenserver::ServerState::from_settings( | |||
| &settings.tokenserver, | |||
| metrics.clone(), | |||
| metrics::metrics_from_opts( | |||
There was a problem hiding this comment.
Tokenserver will have its own separate metrics client -- is that okay? It seems like it shouldn't be a problem to me, but I wanted to double-check
There was a problem hiding this comment.
Yea, it's not a big deal for now.
src/server/metrics.rs
Outdated
| label: String, | ||
| host: Option<String>, |
There was a problem hiding this comment.
not terribly important but this could take references instead. Option::as_deref can easily give you an Option<&str> from the host field Option<String> (as_ref doesn't)
| label: String, | |
| host: Option<String>, | |
| label: &str | |
| host: Option<&str>, |
| @@ -241,7 +245,11 @@ impl Server { | |||
| let tokenserver_state = if settings.tokenserver.enabled { | |||
| Some(tokenserver::ServerState::from_settings( | |||
| &settings.tokenserver, | |||
| metrics.clone(), | |||
| metrics::metrics_from_opts( | |||
There was a problem hiding this comment.
Yea, it's not a big deal for now.
Description
Emits one log line and several useful metrics per Tokenserver request.
Testing
I tested this by running the integration tests with
RUST_LOG=debugand ensuring that the output included the expected log lines. I wasn't sure how to test the metrics.Issue(s)
Closes #1108