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

Stabilize the metrics feature #660

Merged
merged 1 commit into from
May 31, 2024
Merged

Stabilize the metrics feature #660

merged 1 commit into from
May 31, 2024

Conversation

Swatinem
Copy link
Member

Removes the UNSTABLE_ prefix from the feature flag. Also runs a cargo update and updates some dependencies for good measure.

Removes the `UNSTABLE_` prefix from the feature flag.
Also runs a `cargo update` and updates some dependencies for good measure.
@Swatinem Swatinem requested a review from elramen May 29, 2024 09:32
@Swatinem Swatinem self-assigned this May 29, 2024
Copy link
Contributor

@loewenheim loewenheim 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 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"]
Copy link
Contributor

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?

Copy link
Member Author

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.

@Swatinem Swatinem mentioned this pull request May 29, 2024
Copy link
Contributor

@elramen elramen left a 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 😄

Comment on lines +67 to +73
for (i, (k, v)) in self.tags.iter().enumerate() {
if i > 0 {
f.write_char(',')?;
}
write!(f, "{k}:{v}")?;
}
Ok(())
Copy link
Contributor

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()?

Suggested change
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}")

Copy link
Contributor

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:

  1. allocate a String for each tag (the format! call)
  2. collect all those strings into a newly allocated Vec (the collect call)
  3. join the vector into a newly allocated String (the join call)
  4. 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.

Copy link
Member Author

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.

@Swatinem Swatinem merged commit 7ae972e into master May 31, 2024
12 checks passed
@Swatinem Swatinem deleted the swatinem/stable-metrics branch May 31, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants