-
-
Notifications
You must be signed in to change notification settings - Fork 160
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
Stabilize the metrics
feature
#660
Conversation
Removes the `UNSTABLE_` prefix from the feature flag. Also runs a `cargo update` and updates some dependencies for good measure.
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 would honestly almost split this up into two PRs—it has a number of changes that have nothing to do with the metrics feature. It should be fine either way though.
UNSTABLE_metrics = ["sentry-types/UNSTABLE_metrics", "regex"] | ||
UNSTABLE_cadence = ["dep:cadence", "UNSTABLE_metrics"] | ||
metrics = ["sentry-types/metrics", "regex", "crc32fast"] | ||
metrics-cadence1 = ["dep:cadence", "metrics"] |
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.
Out of interest, why is it cadence1
?
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.
probably not too relevant for the SDK because we have semver-breaking changes all the time, but the idea was to be able to support multiple versions of cadence without having to do a semver-breaking bump on our end.
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 don't know much about the transport stuff but the changes to the normalization look good 😄
for (i, (k, v)) in self.tags.iter().enumerate() { | ||
if i > 0 { | ||
f.write_char(',')?; | ||
} | ||
write!(f, "{k}:{v}")?; | ||
} | ||
Ok(()) |
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.
Is it better for performance to do multiple writes vs using collect() and join()?
for (i, (k, v)) in self.tags.iter().enumerate() { | |
if i > 0 { | |
f.write_char(',')?; | |
} | |
write!(f, "{k}:{v}")?; | |
} | |
Ok(()) | |
let res = self | |
.tags | |
.iter() | |
.map(|(k, v)| format!("{}:{}", k, v)) | |
.collect::<Vec<_>>() | |
.join(","); | |
write!(f, "{res}") |
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.
The problem with using format!
, collect
, and join
is that they all allocate. Your version would:
- allocate a
String
for each tag (theformat!
call) - collect all those strings into a newly allocated
Vec
(thecollect
call) - join the vector into a newly allocated
String
(thejoin
call) - write that
String
into the formatter
None of these allocations are necessary. We just want to write some already existing strings into a formatter, separated by commas and colons.
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.
The primary change here was also switching from a HashMap
to a BTreeMap
, to avoid having to pull in the itertools
dependency to sort the map output, as that happens implicitly now.
Removes the
UNSTABLE_
prefix from the feature flag. Also runs acargo update
and updates some dependencies for good measure.