fix: Don't use hidden progress bar #2830
Conversation
src/utils/progress.rs
Outdated
| pub fn is_progress_bar_visible() -> bool { | ||
| env::var("SENTRY_NO_PROGRESS_BAR") != Ok("1".into()) | ||
| pub fn use_progress_bar() -> bool { | ||
| (env::var("SENTRY_NO_PROGRESS_BAR") != Ok("1".into())) || std::io::stdout().is_terminal() |
There was a problem hiding this comment.
We should verify whether stdout or stderr is used for the progress bar
There was a problem hiding this comment.
It uses stderr, changed it.
|
Without this PR we're indeed missing not only the messages emitted on the progress bar itself (with |
|
|
||
| let mut found_files = vec![]; | ||
| let pb = ProgressBar::new_spinner(); | ||
| let mut pb = ProgressBar::new_spinner(); |
There was a problem hiding this comment.
Needs to become mut to handle set_prefix
There was a problem hiding this comment.
Amazing find, thanks so much for the PR!
I added some minor suggestions for the implementation; overall, this is an excellent improvement though!
Side note: it is interesting that we do two assemble endpoints for the sourcemap upload in the "after" logs, as the first assemble request already shows all the chunks are uploaded, the second call is redundant. But, I suppose this is an edge case, so it is fine, but it is interesting behavior that we would not have observed without your change
| if env::var("SENTRY_NO_PROGRESS_BAR") == Ok("1".into()) { | ||
| return false; | ||
| } | ||
| std::io::stderr().is_terminal() |
There was a problem hiding this comment.
I've updated the PR description to also use stderr
| inner.finish_with_message(&msg); | ||
| logging::set_progress_bar(None); | ||
| } else { | ||
| log::info!("> {msg}"); |
There was a problem hiding this comment.
log::info! by default would be hidden (as log level is by default at WARN). I think eprintln! is therefore more suitable here, assuming these final messages typically show in the stderr when running in terminal
| log::info!("> {msg}"); | |
| eprintln!("> {msg}"); |
| } else { | ||
| log::debug!( | ||
| "{}{}", | ||
| self.prefix | ||
| .as_deref() | ||
| .map(|s| format!("{s} ")) | ||
| .unwrap_or_default(), | ||
| msg.as_ref() | ||
| ); | ||
| } |
There was a problem hiding this comment.
m: As this output would not remain visible for long in the terminal, I don't think it makes much sense to print it at all when we don't use a progress bar. Logs at debug level are not visible by default anyways. Unless we find a case where this could be useful, I would omit it.
| } else { | |
| log::debug!( | |
| "{}{}", | |
| self.prefix | |
| .as_deref() | |
| .map(|s| format!("{s} ")) | |
| .unwrap_or_default(), | |
| msg.as_ref() | |
| ); | |
| } | |
| } |
| } else { | ||
| self.prefix = Some(prefix.as_ref().to_owned()); | ||
| } |
There was a problem hiding this comment.
Going along with my previous suggestion, getting rid of logging in the no progress bar case lets us simplify here. We can probably even remove the prefix field from the struct.
| } else { | |
| self.prefix = Some(prefix.as_ref().to_owned()); | |
| } | |
| } |
2 participants
Description
indicatifprogress bar whenSENTRY_NO_PROGRESS_BAR, use no progress bar instead, and just forward messages on it tolog::debug/info.std::io::stderr().is_terminal()to determine whether or not to use a progress bar whenSENTRY_NO_PROGRESS_BARis not specified.log, as ourlogutility ultimately forwards to the progress bar when one is active.Issues
Close #2829
Close CLI-180
Close #2790
Close CLI-171
Testing
sentry-cli debug-files bundle-jvm --log-level=debugstdout and stderr before and after the change:
stdout_before.txt
stdout_after.txt
stderr_before.txt
stderr_after.txt
The stdout remains the same.
The stderr changes, now we see more output, such as the file being currently processed (DEBUG level), and other messages at INFO level, such as
Skipping ./File.java because it is not valid UTF-8which is what motivated this PR in the first place.sentry-cli sourcemaps upload --log-level=debugstdout and stderr before and after the change:
stdout_before.txt
stdout_after.txt
stderr_before.txt
stderr_after.txt
The stdout remains the same as well.
The stderr now shows 2
POSTcalls to/artifactbundle/assemble, in contrast with the singlePOSTcall to that endpoint that we would see without the change.