feat(metrics): lookup/resolver stats - #4411
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4411/docs/iroh/ Last updated: 2026-07-24T12:12:05Z |
| /// Creates a new registry that records resolution outcomes in the given metrics. | ||
| pub fn with_metrics(metrics: Arc<Metrics>) -> Self { | ||
| Self { | ||
| metrics, | ||
| ..Default::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
I can't find any APIs where the user can use this type. As far as I can find it is only returned by some Endpoint::address_lookup and that's it. The rest that is pub is just there for inspection by users.
So I don't think this constructor should be pub.
| /// Returns the metrics tracking per-service resolution outcomes. | ||
| pub fn metrics(&self) -> &Arc<Metrics> { | ||
| &self.metrics | ||
| } |
There was a problem hiding this comment.
This is going to sound very pedantic. The order in which functions are declared in the code is the order they are documented in by rustdoc. This means generally that the most important functions should come first. This is not the most important function.
Yeah I know, neither is set_addr_filter. But let's not make things worse. Can we put it afer len? That is the only other "inspection"-like API on this type so seems like it should belong together.
| /// Number of successful resolutions, labeled by the service that produced the result. | ||
| pub resolve_success: Family<ServiceLabels, Counter>, | ||
| /// Number of failed resolutions, labeled by the service that produced the error. | ||
| pub resolve_error: Family<ServiceLabels, Counter>, |
There was a problem hiding this comment.
These are very confusing. lookups vs resolve? But lookup gets incremented in the resolve() method. IIUC resolve is for results returned and you can get multiple results per lookup. So a lookup is like a request. and resolve is like a yielded item. lookups_failed is a failure to process the lookup request I think.
I'm not saying these as suggestions, I don't really know what to suggest. I'm only trying to understand their meaning.
115bd32 to
07cdb41
Compare
| /// (e.g. once a connection is established) are not counted. | ||
| pub lookups_failed: Counter, | ||
| /// Results yielded per service. | ||
| pub service_results: Family<ServiceLabels, Counter>, |
There was a problem hiding this comment.
this is probably my rust brain. but errors are a sub-set of results. maybe "items" is better? but i admit this is mostly bikeshedding.
There was a problem hiding this comment.
Hmmm items also sounds off. responses?
There was a problem hiding this comment.
in an attempt to find more inspiration i found that the existing docs and APIs literally call these item: https://docs.rs/iroh/latest/iroh/address_lookup/struct.Item.html and https://docs.rs/iroh/latest/iroh/address_lookup/trait.AddressLookup.html#method.resolve
There was a problem hiding this comment.
also, does this need to be singular? does service_results end up as a service_results_count? Probably service_result_count (or service_item_count) makes more sense?
| /// | ||
| /// Includes lookups with no services configured. Lookups abandoned early | ||
| /// (e.g. once a connection is established) are not counted. | ||
| pub lookups_failed: Counter, |
There was a problem hiding this comment.
lookups - lookups_failed ?
There was a problem hiding this comment.
Only roughly correct over time, because it would count inflight lookups too. But maybe good enough. To me it would be clearer to count success too and then inflight is lookups - (failed + success)
3 participants
Description
Adds counters to track lookup success rates and also tracks it by type of resolver.
Breaking Changes
Notes & open questions
Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.