Combine host stats with response stats #1975
Conversation
Previously the two statistics were treated separately. This especially lead to an invalid JSON output, where we showed two separate top-level objects which weren't separated with a comma. Clean up and remove unnecessary abstractions in the process.
| fn format(&self, _stats: ResponseStats) -> Result<Option<String>> { | ||
| Ok(None) | ||
| fn format(&self, _: OutputStats) -> Result<String> { | ||
| Ok(String::new()) |
There was a problem hiding this comment.
Raw does not currently output anything at all on latest release. With the new big per-host feature, on master host statistics are shown as with Compact, but all other stats are hidden, I feel like this doesn't make sense? Is the purpose of the raw mode really to hide all output?
There was a problem hiding this comment.
Yes, the purpose of the raw mode is that the output could be piped into another program for processing. As such, it should not apply any formatting. My current understanding is that we should only print the outcome of the link check itself in raw mode, because the stats would conflate the output and make parsing harder. (I don't suggest that users should parse the raw output, but rather that it could be done as a last resort in case there is no better way to do the data handling.)
There was a problem hiding this comment.
My current understanding is that we should only print the outcome of the link check itself in raw mode
So you mean it should only print 🔍 1 Total (in 0s) ✅ 1 OK 🚫 0 Errors? That would be compact mode. (the default mode) Raw currently does not print anything at all (both with latest release and this PR), except the progress which has nothing to do with the output format.
I currently do not see any use in this raw mode. If the purpose is to print nothing at all, I think we could say that's the users responsibility. ( > /dev/null) Also the name doesn't make sense. Raw doesn't sound like "print nothing at all".
There was a problem hiding this comment.
Raw currently does not print anything at al
Oh! My bad. That's certainly unexpected. In this case, your change makes sense. I don't know if it broke or if it has always been the case. Too lazy to check, but my hope is that it did something useful at some point. ^^ So the original idea of was "print all the check results without any special formatting; just 'plaintext'."
There was a problem hiding this comment.
Ok, I've just double-checked. It was introduced in 8c0a32d. Even back then final stats were hidden with raw mode. (format_stats returned None) The only difference was in the ResponseFormatter (write_response) which is used for printing progress (not the final result).
This is different by now. We now have mode to specify how progress formatting is done. (e.g. emoji)
Because of this I would like to fully remove raw mode. It serves no purpose except hiding the final output. But this equivalent with e.g. piping to /dev/null.
There was a problem hiding this comment.
I've created #1976
This PR retains the current behaviour of hiding the results. We can then discuss what to do about the raw format on the new issue.
thomas-zahner
force-pushed
the
cleanup-host-stats-formatting
branch
from
96cf922 to
3903b26
Compare
thomas-zahner
force-pushed
the
cleanup-host-stats-formatting
branch
from
3903b26 to
483c88b
Compare
|
Yeah, my bad. It looks like raw mode is broken. See my other comment. The gist of it is, we can and should fix it and I'm okay with your change. 😊 |
2 participants
Previously the two statistics were treated separately. This especially lead to an invalid JSON output, where we showed two separate top-level objects which weren't separated with a comma. Clean up and remove unnecessary abstractions in the process.
Previous
{ "total": 12, "successful": 8, "unknown": 0, "unsupported": 0, "timeouts": 0, "redirects": 0, "excludes": 4, "errors": 0, "cached": 0, "success_map": {}, "error_map": {}, "suggestion_map": {}, "redirect_map": {}, "excluded_map": {}, "duration_secs": 0, "detailed_stats": false } { "host_statistics": { "endler.dev": { "cache_hit_rate": 0.0, "cache_hits": 0, "cache_misses": 1, "client_errors": 0, "median_request_time_ms": 72, "rate_limited": 0, "server_errors": 0, "status_codes": { "200": 1 }, "success_rate": 1.0, "successful_requests": 1, "total_requests": 1 } } }New
{ "total": 12, "successful": 8, "unknown": 0, "unsupported": 0, "timeouts": 0, "redirects": 0, "excludes": 4, "errors": 0, "cached": 0, "success_map": {}, "error_map": {}, "suggestion_map": {}, "redirect_map": {}, "excluded_map": {}, "duration_secs": 0, "detailed_stats": false, "host_stats": { "endler.dev": { "total_requests": 1, "successful_requests": 1, "success_rate": 1.0, "rate_limited": 0, "client_errors": 0, "server_errors": 0, "median_request_time_ms": 87, "cache_hits": 0, "cache_misses": 1, "cache_hit_rate": 0.0, "status_codes": { "200": 1 } } } }