Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rust): add some metrics backends #4063

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dbanda
Copy link
Contributor

@dbanda dbanda commented Apr 21, 2023

Switches the metrics client to be a trait and introduces a testing and datadog backend of the metrics client. Datadog is still WIP but this impl uses the dogstatsd library.

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a29e615) 90.34% compared to head (c747139) 90.34%.

❗ Current head c747139 differs from pull request most recent head 14d6aa8. Consider uploading reports for the commit 14d6aa8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4063   +/-   ##
=======================================
  Coverage   90.34%   90.34%           
=======================================
  Files         803      803           
  Lines       39514    39514           
  Branches      245      245           
=======================================
  Hits        35698    35698           
  Misses       3784     3784           
  Partials       32       32           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dbanda dbanda force-pushed the dbanda/rust/add_metrics branch 2 times, most recently from 118a232 to 506ca85 Compare May 23, 2023 05:50
@dbanda dbanda marked this pull request as ready for review May 23, 2023 05:57
@dbanda dbanda requested a review from a team as a code owner May 23, 2023 05:57
Comment on lines 56 to 62
for _ in 0..v.abs() {
if v < 0 {
self.client_sd.decr(key, &tags_str).unwrap();
} else {
self.client_sd.incr(key, &tags_str).unwrap();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this logic be replaced by https://docs.rs/dogstatsd/latest/dogstatsd/struct.Client.html#method.count ?

Seems inefficient to send n decr messages vs. subtract n if that's possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was so dumb. Updated.

@lynnagara lynnagara requested a review from untitaker July 14, 2023 17:16
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would recommend using cadence instead of dogstatsd, and take a look at relay for how it is set up there

@@ -8,7 +8,12 @@ use std::collections::HashMap;
use std::net::{ToSocketAddrs, UdpSocket};
use std::sync::Arc;

pub trait Metrics {
pub trait MetricsClientTrait: Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not idiomatic to name traits as Trait

@getsantry getsantry bot added the Stale label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants