chore: Move to metrics-exporter-statsd #212
Conversation
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Please consider addressing the comments before merging
| typing: | ||
| name: "mypy typing" | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 |
There was a problem hiding this comment.
mypy takes longer than 5 minutes now. I don't know why but this timeout just made the PR fail.
|
|
||
| match metric.ty { | ||
| MetricType::Counter => { | ||
| let counter = metrics::counter!(key, &labels); |
There was a problem hiding this comment.
What does this macro do ? It seems it would be executed every time we produce a metric.
There was a problem hiding this comment.
that's what it does, produce metrics. it's part of the metrics crate. what it does internally (blocking udp send or preaggregate, or async) is an impl detail of the metrics crate, but it sure seems fast.
sentry_streams/src/metrics.rs
Outdated
| let addr_str = format!("{}:{}", host, port); | ||
| let socket_addrs: Vec<SocketAddr> = match addr_str.to_socket_addrs() { | ||
| Ok(addrs) => addrs.collect(), | ||
| Err(e) => { | ||
| error!("Failed to resolve metrics address {}: {}", addr_str, e); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if init(recorder).is_err() { | ||
| warn!("Warning: Metrics recorder already initialized, skipping"); | ||
| let statsd_addr = match socket_addrs.first() { | ||
| Some(addr) => *addr, | ||
| None => { | ||
| error!("Could not resolve metrics address into a socket address"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
IS this all meant to just turn the host into an ip address ?
There was a problem hiding this comment.
yes it was bullshit from earlier attempts to get the thing working. not necessary
| recorder = recorder.with_tag(key, value); | ||
| } | ||
| } | ||
| let host = metric_config.host(); |
There was a problem hiding this comment.
I think we should find a way to unit test this logic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| .tags | ||
| .iter() | ||
| .map(|(k, v)| { | ||
| let key = k.map(|k| format!("{}", k)).unwrap_or_default(); |
There was a problem hiding this comment.
Bug: Empty tag key when metric tag key is None
When a metric tag has a None key, unwrap_or_default() produces an empty string "" as the tag key. This results in metrics being sent with empty tag keys, which is invalid for most metrics backends (including statsd/Datadog). Empty tag keys can cause parsing errors, data corruption, or silent metric dropping. Tags with None keys could be filtered out entirely instead.
2 participants
Improves performance over synchronous syscalls on the main thread.