-
-
Notifications
You must be signed in to change notification settings - Fork 57
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(metrics-summaries): Parse only the minimum to see if we need to skip the message #5375
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5375 +/- ##
==========================================
- Coverage 90.71% 90.62% -0.09%
==========================================
Files 881 881
Lines 42835 42835
Branches 288 288
==========================================
- Hits 38857 38820 -37
- Misses 3936 3973 +37
Partials 42 42 ☔ View full report in Codecov by Sentry. |
} | ||
|
||
#[derive(Debug, Default, Deserialize)] | ||
struct InputMessage { | ||
#[serde(default)] | ||
_metrics_summary: BTreeMap<String, Vec<FromMetricsSummary>>, | ||
_metrics_summary: Option<Box<RawValue>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe IgnoredAny might fit your purpose even more narrowly -- it isn't specific to serde-json
, and AFAIK there's fewer hacks in there because it doesn't allow you to access the inner value as a string and really just discards. who knows if it is faster though
project_id: u64, | ||
received: f64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this mandatory field is new, right? nevermind, i think this is the same kind of breakage as #5367 which we fixed, so it should be fine
|
||
if msg._metrics_summary.is_none() { | ||
return Ok(InsertBatch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can shorten it to Ok(InsertBatch::default())
, or i think in the generic-metrics PR there's an alias InsertBatch::skip()
as well
thiserror = "1.0" | ||
tokio = { version = "1.19.2", features = ["full"] } | ||
statsdproxy = { version = "0.1.0", features = ["cadence-adapter"], git = "https://github.com/getsentry/statsdproxy", default-features = false } | ||
statsdproxy = { version = "0.1.0", features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revert the formatting changes here? I feel that between various people, this file gets reformatted a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I don’t think this is worth the complexity. Parsing a bunch of trivial number types should not be as bad for perf.
If it really is, might be better to just stop emitting such messages from the source?
As pointed out, not worth the effort, I'll focus on emitting the summaries on a different topic. |
No description provided.