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

Add metrics tracking number of recently loaded crates/versions/platforms #1019

Merged
merged 10 commits into from
Oct 23, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Aug 30, 2020

These metrics are intended to be useful for evaluating changes for #1004. Knowing how many different crates get accessed over short timespans could help us know things like the potential memory cost of keeping cached indexes around short term.

I'm not sure what the performance impact of these might be in production. The most likely issue would be a large memory usage if the number of different platforms requested per-hour is very high, leading to a lot of small strings kept around to track these.

(based on #1017, so to review the lockfile changes look at the last commit in isolation)

@jyn514 jyn514 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed A-backend Area: Webserver backend labels Aug 30, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 30, 2020

The most likely issue would be a large memory usage if the number of different platforms requested per-hour is very high, leading to a lot of small strings kept around to track these.

We already run high at about 5 GB in prod. I find it hard to think the number of crate/version/platform pairs would take up anything close to that, do you have a guesstimate for 'lot of small strings'?

src/metrics/mod.rs Outdated Show resolved Hide resolved
@pietroalbini
Copy link
Member

I think we can avoid the problem with strings by not storing them at all. My understanding is we don't care about the crate names/versions, so we can just store i32s:

  • For crates we could map the crate ID with the instant.
  • For versions we could map the release ID with the instant.
  • For targets we could map (release ID, interned target name) with the instant.

@jyn514

This comment has been minimized.

@Nemo157 Nemo157 force-pushed the release-metrics branch 2 times, most recently from a379c08 to bb75349 Compare August 31, 2020 18:15
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Didn't look too closely, but the idea seems right. Might review again later.

build.rs Show resolved Hide resolved
src/metrics/mod.rs Outdated Show resolved Hide resolved
src/metrics/mod.rs Outdated Show resolved Hide resolved
@pietroalbini
Copy link
Member

Other than Joshua's comments, this looks good to me!

src/metrics/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/metrics/mod.rs Outdated Show resolved Hide resolved
src/metrics/mod.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 5, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 14, 2020

Looks good to me after a rebase :)

@Nemo157 Nemo157 force-pushed the release-metrics branch 2 times, most recently from 5bee3a0 to 22ae1da Compare October 22, 2020 19:23
Copy link
Member Author

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

Rebased, some minor conflicts but I've identified the only major conflict below, both this and master had started tracking the target when extracting that from the URL but this wants it without trailing /.

String::new()
} else {
format!("{}/", target)
};
Copy link
Member Author

Choose a reason for hiding this comment

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

this target handling was the only major conflict.

@jyn514 jyn514 merged commit 394e506 into rust-lang:master Oct 23, 2020
@Nemo157 Nemo157 deleted the release-metrics branch October 31, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend Area: Webserver backend S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants