feat: add more metrics #130
Conversation
Adds richer set of metrics and metric handling Closes #120
src/server/location.rs
Outdated
| } else { | ||
| metrics.incr_with_tags( | ||
| "location.unknown.city", | ||
| Some(&Tags::from_extra(vec![("ip", ip_addr.to_string())])), |
There was a problem hiding this comment.
extras are only sent to sentry, so all these one offs in this module are unnecessary for metrics calls (I guess that applies to the new from_extra method too)
There was a problem hiding this comment.
Yeah, I debated adding these as proper tags, but I figured we'd hit cardinality issues pretty fast. I'll remove the tag from this for now.
As for keeping the from::extra, I can see that potentially being useful in the future.
| key, | ||
| Tiles { | ||
| json: tiles, | ||
| ttl: SystemTime::now() + Duration::from_secs(settings.tiles_ttl as u64), |
There was a problem hiding this comment.
I'm debating if we should make this TTL * 2, to cover potential issues with ADM returning nothing, or an error. To be fair, we should probably also exit if adM returns anything other than a 200, and assume that they're having problems.
There was a problem hiding this comment.
Initially I leaned towards hiding things from the client like this but #92 suggests the client wants more feedback from the other end.
There was a problem hiding this comment.
So, if a background refresh fails, how should we notify the client?
src/adm/filter.rs
Outdated
| @@ -238,16 +244,19 @@ impl AdmFilter { | |||
| }; | |||
| if let Err(e) = self.check_advertiser(adv_filter, &mut tile, tags) { | |||
| dbg!("bad adv"); | |||
| metrics.incr_with_tags("tag.err.invalid_advertiser", Some(tags)); | |||
There was a problem hiding this comment.
what does "tag" in this name mean?
Also related, dunno if you saw this comment: #130 (review)
There was a problem hiding this comment.
ah, I think that was the product of a bad replace.
I'll correct the names (and I'll also add adm)
| key, | ||
| Tiles { | ||
| json: tiles, | ||
| ttl: SystemTime::now() + Duration::from_secs(settings.tiles_ttl as u64), |
There was a problem hiding this comment.
Initially I leaned towards hiding things from the client like this but #92 suggests the client wants more feedback from the other end.
…e into feat/120-metrics
…e into feat/120-metrics
Adds richer set of metrics and metric handling
Closes #120
Description
Added more metric information for contile including:
Testing
Testing should result in increase of various metrics.
Issue(s)
Closes #120
┆Issue is synchronized with this Jira Bug