Show transfer rate when fetching/updating registry index #9395

weihanglo · 2021-04-23T14:17:07Z

Possibly fixes #8483.

To avoid blinking too frequently, update rate is throttled by one second.

I am not sure how to write tests for it 😂

Updated (2020-04-28)

Current looking

    Updating crates.io index
       Fetch [==>                      ]  14.50%, 258.45KiB/s
       
    Updating crates.io index
       Fetch [======>                  ]  40.50%, (1234/282342) resolving deltas

rust-highfive · 2021-04-23T14:17:10Z

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

alexcrichton

This looks quite nice, thanks for this!

It's ok to not have tests for this, most of Cargo's human-readable terminal UI doesn't have many tests since it's so difficult to do in CI. We as reviewers will check this out to poke around at it and test it manually!

src/cargo/sources/git/utils.rs

weihanglo · 2021-04-23T19:25:40Z

All updated.

Now it looks like below:

    Updating crates.io index
       Fetch [==>                      ]  14.50%, 258.45KiB/s
       
    Updating crates.io index
       Fetch [======>                  ]  40.50%, (1234/282342) resolving deltas

src/cargo/sources/git/utils.rs

alexcrichton · 2021-04-27T14:15:14Z

@rfcbot fcp merge

This affects's Cargo's default UI, so I want to be sure to get sign-off from others as well.

rfcbot · 2021-04-27T14:15:15Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

alexcrichton · 2021-04-27T18:17:23Z

src/cargo/sources/git/utils.rs

+                    // Receiving objects.
+                    let now = Instant::now();
+                    // Scrape a `received_bytes` to the counter every 300ms.
+                    if now - last_update > Duration::from_millis(300) {


As one thought I had while reading this, do you know if libgit2 is guaranteed to call this callback perioidically? If we receive data and then don't receive anything else for 10s, does that mean the transfer rate is "constant" for that 10s even though we don't actually receive anything?

(basically I think for the transfer rate to be accurate we would have to guarantee that this transfer_progress callback is called periodically even if the network is idle.

It seems that libgit2 does not periodically call the callback. So yes it maybe be stuck. Git CLI has a similar issue too. IIRC all Git operations share the same timeout config from the same curl handle, so its nearly impossible to reuse it. Therefore we have some options:

Should we implement another timeout logic to update the transfer rate?

Or do we accept a "constant" transfer rate while encountering network issue?

Is there any other possible solution to handle it?

I think our options are pretty limited here. One option is to remove the download rate (boo) and the other one would be to add some sort of threading that calls the update on a tick. I don't think those are really worth it for now though. Perhaps you can leave a comment here to this effect and we can tackle this in the future if it's actually an issue?

Comment added. Also fixed an unit display issue 😂
Thanks for your suggestion!

rfcbot · 2021-04-28T19:53:28Z

🔔 This is now entering its final comment period, as per the review above. 🔔

ehuss · 2021-04-28T20:01:40Z

This looks great!

One question I had unrelated to this PR: I have noticed that the progress bar does not appear if libcurl is being used. For example, if you set the environment variable HTTP_TIMEOUT=5, then cargo uses the libcurl backend, which seems to prevent the progress bar from appearing during the download. I don't suppose while looking at this if you maybe saw an obvious reason for that?

weihanglo · 2021-04-29T10:49:02Z

This looks great!

One question I had unrelated to this PR: I have noticed that the progress bar does not appear if libcurl is being used. For example, if you set the environment variable HTTP_TIMEOUT=5, then cargo uses the libcurl backend, which seems to prevent the progress bar from appearing during the download. I don't suppose while looking at this if you maybe saw an obvious reason for that?

I also noticed that and did some quick tests. Problem probably is not related to cargo. It can be reproduced by registering a simple git2-curl backend to git2's fetch example without any configurations. I'll try to investigate the issue after this one.

alexcrichton · 2021-04-30T14:12:34Z

@bors: r+

bors · 2021-04-30T14:12:36Z

📌 Commit e4d4347 has been approved by alexcrichton

bors · 2021-04-30T14:12:44Z

⌛ Testing commit e4d4347 with merge 6701e33...

bors · 2021-04-30T14:37:20Z

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 6701e33 to master...

Update cargo 7 commits in 4369396ce7d270972955d876eaa4954bea56bcd9..f3e13226d6d17a2bc5f325303494b43a45f53b7f 2021-04-27 14:35:53 +0000 to 2021-04-30 21:50:27 +0000 - Fix problem with metrics test. (rust-lang/cargo#9440) - Show transfer rate when fetching/updating registry index (rust-lang/cargo#9395) - Fix collision doc tests randomly failing. (rust-lang/cargo#9434) - Add missing tracking issues and unstable docs. (rust-lang/cargo#9429) - Fix dep-info files emitting paths relative to deps' roots (rust-lang/cargo#9421) - Upgrade to GitHub-native Dependabot (rust-lang/cargo#9428) - Only deny the `unused_mut` lint (rust-lang/cargo#9425)

weihanglo · 2021-05-01T10:03:47Z

@ehuss I've created an issue #9444 for the disappearance of progress bar.

pickfire · 2021-05-12T10:56:01Z

@weihanglo can you please show an example of the same progress bar with terminal width 80? I believe some people are still using that (including me). Does it still works nicely?

weihanglo · 2021-05-12T11:53:12Z

@pickfire
Here are some examples on 80x24 terminal. The "resolving deltas" part is truncated, which is acceptable for me.

weihanglo added 2 commits April 23, 2021 21:29

refactor: progress.tick support attaching message

77d993c

feat: show transfer rate and total bytes received

d58aa8a

rust-highfive assigned Eh2406 Apr 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2021

weihanglo changed the title ~~Show transfer progress when fetching/updating registry index~~ Show transfer rate when fetching/updating registry index Apr 23, 2021

alexcrichton reviewed Apr 23, 2021

View reviewed changes

src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved

src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved

src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved

weihanglo added 3 commits April 24, 2021 02:11

fix: remove unnecessary progress tick rate limiting

7e3f7d6

fix: remove total bytes received from progress message

507d7e4

fix: message of progress when resolving deltas

f2172db

alexcrichton reviewed Apr 23, 2021

View reviewed changes

src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved

weihanglo added 2 commits April 27, 2021 14:23

feat: add metrics counter utility

a813ba0

refactor: use metrics counter as an abstaction

de84f35

weihanglo force-pushed the issue-8483 branch from c25d6cf to de84f35 Compare April 27, 2021 06:23

alexcrichton reviewed Apr 27, 2021

View reviewed changes

src/cargo/sources/git/utils.rs Outdated Show resolved Hide resolved

alexcrichton added the T-cargo Team: Cargo label Apr 27, 2021

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 27, 2021

refactor: pass Instant to MetricsCounter

a89b1e8

alexcrichton reviewed Apr 27, 2021

View reviewed changes

weihanglo added 2 commits April 29, 2021 00:58

fix: bytes per second should not prefix with i

9df531b

comment about caveat of current transfer rate refresh

e4d4347

rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 28, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2021

bors merged commit 6701e33 into rust-lang:master Apr 30, 2021

weihanglo deleted the issue-8483 branch April 30, 2021 15:24

ehuss mentioned this pull request Apr 30, 2021

Update cargo rust-lang/rust#84765

Merged

weihanglo mentioned this pull request May 1, 2021

[git2-curl] The progress bar won't appear when using libcurl as git2 backend #9444

Open

rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels May 8, 2021

ehuss added this to the 1.54.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show transfer rate when fetching/updating registry index #9395

Show transfer rate when fetching/updating registry index #9395

weihanglo commented Apr 23, 2021 •

edited

Loading

rust-highfive commented Apr 23, 2021

alexcrichton left a comment

weihanglo commented Apr 23, 2021

alexcrichton commented Apr 27, 2021

rfcbot commented Apr 27, 2021 •

edited by ehuss

Loading

alexcrichton Apr 27, 2021

weihanglo Apr 28, 2021

alexcrichton Apr 28, 2021

weihanglo Apr 28, 2021

rfcbot commented Apr 28, 2021

ehuss commented Apr 28, 2021

weihanglo commented Apr 29, 2021

alexcrichton commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented Apr 30, 2021

weihanglo commented May 1, 2021

pickfire commented May 12, 2021

weihanglo commented May 12, 2021

Show transfer rate when fetching/updating registry index #9395

Show transfer rate when fetching/updating registry index #9395

Conversation

weihanglo commented Apr 23, 2021 • edited Loading

Updated (2020-04-28)

rust-highfive commented Apr 23, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

weihanglo commented Apr 23, 2021

alexcrichton commented Apr 27, 2021

rfcbot commented Apr 27, 2021 • edited by ehuss Loading

alexcrichton Apr 27, 2021

Choose a reason for hiding this comment

weihanglo Apr 28, 2021

Choose a reason for hiding this comment

alexcrichton Apr 28, 2021

Choose a reason for hiding this comment

weihanglo Apr 28, 2021

Choose a reason for hiding this comment

rfcbot commented Apr 28, 2021

ehuss commented Apr 28, 2021

weihanglo commented Apr 29, 2021

alexcrichton commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented Apr 30, 2021

weihanglo commented May 1, 2021

pickfire commented May 12, 2021

weihanglo commented May 12, 2021

weihanglo commented Apr 23, 2021 •

edited

Loading

rfcbot commented Apr 27, 2021 •

edited by ehuss

Loading