log all QNetworkReply error messages at debug level #3732
Conversation
levkropp
force-pushed
the
log-network-errors
branch
2 times, most recently
from
c011839
to
ad458a5
Compare
levkropp
force-pushed
the
log-network-errors
branch
4 times, most recently
from
5d58224
to
770f789
Compare
src/network/url_downloader.cpp
Outdated
| // Log the original error message at debug level | ||
| mpl::log(mpl::Level::debug, | ||
| category, | ||
| fmt::format("Qt error {}: {}", static_cast<int>(error_code), error_string)); |
There was a problem hiding this comment.
Are you aware of mp::utils::qenum_to_string?
src/network/url_downloader.cpp
Outdated
| fmt::format("Qt error {}: {}", static_cast<int>(error_code), error_string)); | ||
|
|
||
| const bool is_timeout_error = (error_code == QNetworkReply::TimeoutError); | ||
| const auto msg = is_timeout_error ? "Network timeout" : error_string; |
There was a problem hiding this comment.
Do we still need to replace Qt's timeout message with "Network timeout"?
src/network/url_downloader.cpp
Outdated
|
|
||
| if (is_timeout_error && download_timeout.isActive()) | ||
| { | ||
| mpl::log(mpl::Level::debug, category, "Timeout error detected but download_timeout is still active"); |
There was a problem hiding this comment.
Would this indicate an error in our logic? Or in what circumstances is it expected to occur?
There was a problem hiding this comment.
The line used to be
const auto msg = download_timeout.isActive() ? reply->errorString().toStdString() : "Network timeout";
So this line is to check if TimeoutErrors happen even when download_timeout.isActive() is true to potentially shed some light on the "instant timeouts" issue and see if its related to QTimer
Note however that the "timeouts" we are getting in #3662 are from us calling abort() which throw an OperationCanceledError with "Operation canceled" as the error string.
https://doc.qt.io/qt-6/qnetworkreply.html#NetworkError-enum
So this is irrelevant for resolving that issue.
There was a problem hiding this comment.
Should this be a warning though?
There was a problem hiding this comment.
Should this be a warning though?
I guess it can be at error level, but this should never be triggered by an OperationCanceledError and doesn't have any impact on the rest of the error handling which is why I originally put it as debug
levkropp
force-pushed
the
log-network-errors
branch
from
770f789
to
0ca1610
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3732 +/- ##
=======================================
Coverage 88.86% 88.87%
=======================================
Files 256 256
Lines 14569 14579 +10
=======================================
+ Hits 12947 12957 +10
Misses 1622 1622 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
levkropp
force-pushed
the
log-network-errors
branch
4 times, most recently
from
e11c6be
to
027d372
Compare
src/network/url_downloader.cpp
Outdated
|
|
||
| if (download_timeout.isActive()) | ||
| { | ||
| mpl::log(mpl::Level::debug, category, "download_timeout is still active"); | ||
| } |
There was a problem hiding this comment.
| if (download_timeout.isActive()) | |
| { | |
| mpl::log(mpl::Level::debug, category, "download_timeout is still active"); | |
| } | |
| mpl::log(mpl::Level::debug, category, fmt::format("download_timeout is {}active", download_timeout.isActive() ? "" : "in")); |
levkropp
force-pushed
the
log-network-errors
branch
2 times, most recently
from
079cb01
to
aae279b
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
|
@levkropp Merge conflicts |
levkropp
force-pushed
the
log-network-errors
branch
from
8ec5b61
to
92d559e
Compare
levkropp
force-pushed
the
log-network-errors
branch
from
92d559e
to
cc8d248
Compare
The enum for timeouts in Qt's networking library is
QNetworkReply::TimeoutError. Different logs are printed at debug, warning, and error levels.MULTI-1529