Optionally show progress while signing. #35

ximon18 · 2024-11-25T14:50:34Z

Signing can be a slow operation.

Prior to this PR there is no feedback at all when signing, one doesn't even know if the signing is stuck or going so slowly that you'd rather stop it than wait.

This PR uses support in domain to know how far operations have progressed in combination with the indicatif crate to output helpful progress bars.

Note: This is quick proof-of-concept, it is quite messy at the moment, hence the draft status.

Example appearance:

…ss-while-signing

tertsdiepraam

This is very cool, but I have different taste in a few aspects apart from the small issues I noted below.

So these are just my personal preferences stated as fact:

This should be a MultiProgress with multiple steps. And we should show at which step it is.
The progress bars should not be cleared.
Progress bars should not be reused with different messages.
Spinners should be used for steps where we don't know how long it takes.

tertsdiepraam · 2024-11-25T18:44:27Z

src/commands/signzone.rs

+                if let Some(log) = log {
+                    writeln!(
+                        log,
+                        "Loaded {len} records from {} [{} bytes] in {}",


I think the unit of len here is not records, but bytes.

But records.len() or something similar should work.

I can't see the surrounding code on my phone via the GH app.. I think there was one place where I thought the same but was wrong, maybe you are right, but I can't check right now.

tertsdiepraam · 2024-11-25T18:47:57Z

src/commands/signzone.rs

                }
            }
        }

+        if let Some(pb) = &pb {
+            pb.set_message("Sorting");


Might be best to create a new unbounded progress bar here. I'm not seeing this update for some reason.

tertsdiepraam · 2024-11-25T19:07:23Z

src/commands/signzone.rs

@@ -474,20 +492,48 @@ impl SignZone {
        let (apex, ttl) = Self::find_apex(&records).unwrap();

        // Hash the zone with NSEC or NSEC3.
+        let pb = if matches!(&log, Some(log) if log.is_terminal() && self.progress) {
+            let pb = ProgressBar::new(0).with_message("Hashing");


This should probably be ProgressBar::no_length() and then with slightly different parsing. Also we should call tick() to make this actually show up, because it doesn't seem to do that by itself.

I tried calling tick() but that also didn't help.

tertsdiepraam · 2024-11-25T19:12:19Z

src/commands/signzone.rs

+            let pb = ProgressBar::new(num_families as u64);
+            pb.set_draw_target(ProgressDrawTarget::stderr_with_hz(1));
+            pb.set_style(
+                ProgressStyle::with_template(PROGRESS_STYLE)
+                    .unwrap()
+                    .with_key("eta", |state: &ProgressState, w: &mut dyn Write| {
+                        write!(w, "{:.1}s", state.eta().as_secs_f64()).unwrap()
+                    })
+                    .progress_chars("#>-"),
+            );


This style should probably be saved somewhere so we can reuse it.

Yup, there's far too much repetitive boilerplate here, but not being familiar with the progress bar crate I wanted to first get something working.

tertsdiepraam · 2024-11-25T19:29:40Z

src/commands/signzone.rs

+                        if let Some(pb) = &pb {
+                            if inc_len > 0 {
+                                pb.inc_length(inc_len as u64);
+                            }
+                            if inc_pos > 0 {
+                                pb.inc(inc_pos as u64);
+                            }
+                            if let Some(new_phase) = new_phase {
+                                pb.set_message(new_phase);
+                            }


This also feels like it should create new progress bars or something.

ximon18 · 2024-11-25T19:35:35Z

This is very cool, but I have different taste in a few aspects apart from the small issues I noted below.

So these are just my personal preferences stated as fact:

This should be a MultiProgress with multiple steps. And we should show at which step it is.

The progress bars should not be cleared.

Progress bars should not be reused with different messages.

Spinners should be used for steps where we don't know how long it takes.

Thx:

MultiProgress seemed to me (a) to be about concurrent actions which these are not, and (b) likely has some overhead to be thread safe, so I chose not to use it for those reasons.
Keeping the progress bar.. I'm not sure. I find the summary afterward more useful and leaving a wide bar across your screen that no longer updates to tell you anything new a bit annoying.
The different messages are phases of a single task with progress.
I wondered maybe "Sorting" isn't appearing because the sorting operation is so CPU intensive, no I/O or anything, but didn't look into it yet. I wasn't aware of a pure spinner, I wondered about a left-right-left-right bouncing progress bar, or .. don't know, ideally sorting would also report progress as it can be slow on a big zone.

Btw, what does it mean to state preference as fact? 🤪

ximon18 · 2024-11-25T19:37:47Z

Btw, the large repetitive progress bar construction should be factored out, the code is v messy at present, but I didn't want to spend time on that yet.

tertsdiepraam · 2024-11-25T19:39:30Z

Btw, what does it mean to state preference as fact? 🤪

I just meant that I phrased without prefixing everything with "I think" :)

Keeping the progress bar.. I'm not sure. I find the summary afterward more useful and leaving a wide bar across your screen that no longer updates to tell you anything new a bit annoying.

Makes sense yeah. It's kind of satisfying to me, but I understand.

I wondered maybe "Sorting" isn't appearing because the sorting operation is so CPU intensive, no I/O or anything, but didn't look into it yet. I wasn't aware of a pure spinner, I wondered about a left-right-left-right bouncing progress bar, or .. don't know, ideally sorting would also report progress as it can be slow on a big zone.

I would work with steady_tick enabled and as a spinner. The bouncy thing would be cool but I don't think indicatif has that out of the box.

The different messages are phases of a single task with progress.

Hmmm, but then we should know the total up front right? And maybe the message should then be {main_task}: {subtask}

ximon18 · 2024-11-26T08:09:28Z

Btw, what does it mean to state preference as fact? 🤪

I just meant that I phrased without prefixing everything with "I think" :)

I'm glad I asked. I took it to mean that while it is your preference you believe it not to just be a preference but to be a fact that it should be done that way ;-)

ximon18 · 2024-11-26T08:09:46Z

Keeping the progress bar.. I'm not sure. I find the summary afterward more useful and leaving a wide bar across your screen that no longer updates to tell you anything new a bit annoying.

Makes sense yeah. It's kind of satisfying to me, but I understand.

I will try it out and see what I think of it.

Initial hack to add progress reporting using new support in domain.

9f99de5

ximon18 requested a review from a team November 25, 2024 14:50

ximon18 added 3 commits November 25, 2024 15:53

Merge branch 'use-parallelized-sorting-to-go-faster' into show-progre…

719d719

…ss-while-signing

Merge branch 'use-parallelized-sorting-to-go-faster' into show-progre…

39579fa

…ss-while-signing

Remove wrongly committed change.

dce0d65

ximon18 mentioned this pull request Nov 25, 2024

Report zonefile parsing & signing progress to the caller. NLnetLabs/domain#448

Draft

tertsdiepraam reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally show progress while signing. #35

Optionally show progress while signing. #35

ximon18 commented Nov 25, 2024 •

edited

Loading

tertsdiepraam left a comment

tertsdiepraam Nov 25, 2024

tertsdiepraam Nov 25, 2024

ximon18 Nov 25, 2024

tertsdiepraam Nov 25, 2024

tertsdiepraam Nov 25, 2024

ximon18 Nov 25, 2024

tertsdiepraam Nov 25, 2024

ximon18 Nov 25, 2024

tertsdiepraam Nov 25, 2024

ximon18 commented Nov 25, 2024 •

edited

Loading

ximon18 commented Nov 25, 2024

tertsdiepraam commented Nov 25, 2024 •

edited

Loading

ximon18 commented Nov 26, 2024

ximon18 commented Nov 26, 2024

Optionally show progress while signing. #35

Are you sure you want to change the base?

Optionally show progress while signing. #35

Conversation

ximon18 commented Nov 25, 2024 • edited Loading

tertsdiepraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximon18 commented Nov 25, 2024 • edited Loading

ximon18 commented Nov 25, 2024

tertsdiepraam commented Nov 25, 2024 • edited Loading

ximon18 commented Nov 26, 2024

ximon18 commented Nov 26, 2024

ximon18 commented Nov 25, 2024 •

edited

Loading

ximon18 commented Nov 25, 2024 •

edited

Loading

tertsdiepraam commented Nov 25, 2024 •

edited

Loading