-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display the correct failing host on worker boot #5352
base: master
Are you sure you want to change the base?
Display the correct failing host on worker boot #5352
Conversation
When a worker cannot establish a websocket connection the log line reports the URL of the REST API, and not the websocket endpoint where the connection failed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5352 +/- ##
==========================================
- Coverage 98.32% 98.31% -0.01%
==========================================
Files 389 389
Lines 37319 37321 +2
==========================================
Hits 36693 36693
- Misses 626 628 +2 ☔ View full report in Codecov by Sentry. |
So the next learning opportunity for you: Extend the tests to make the coverage check https://github.com/os-autoinst/openQA/pull/5352/checks?check_run_id=18141921282 happy :) |
I tried but got stuck on getting connection refused and not the problem at hand 😅 |
@@ -157,6 +157,11 @@ sub _setup_websocket_connection ($self, $websocket_url = undef) { | |||
|
|||
my $error = $tx->error; | |||
my $error_message = "Unable to upgrade to ws connection via $websocket_url"; | |||
|
|||
if ($tx->res->error && $tx->res->error->{message}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the post-if to be consistent with other lines
if ($tx->res->error && $tx->res->error->{message}) { | ||
$error_message .= sprintf(", %s in %s", $tx->res->error->{message}, $tx->req->url); | ||
} | ||
|
||
$error_message .= ", code $error->{code}" if ($error && $error->{code}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use $error
for simplicity and there should always be a message:
if ($tx->res->error && $tx->res->error->{message}) { | |
$error_message .= sprintf(", %s in %s", $tx->res->error->{message}, $tx->req->url); | |
} | |
$error_message .= ", code $error->{code}" if ($error && $error->{code}); | |
if ($error) { | |
$error_message .= ", $error->{message}"; | |
$error_message .= ", code $error->{code}" if $error->{code}; | |
} |
I have also removed the URL because it is already part of the error message and I would just change that other part. However, if you disagree and want the original URL still being logged that's fine as well.
@@ -157,6 +157,11 @@ sub _setup_websocket_connection ($self, $websocket_url = undef) { | |||
|
|||
my $error = $tx->error; | |||
my $error_message = "Unable to upgrade to ws connection via $websocket_url"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just alter the URL here:
my $error_message = "Unable to upgrade to ws connection via $websocket_url"; | |
my $error_message = 'Unable to upgrade to ws connection via ' . $tx->req->url; |
The original URL is still logged in the "Establishing …" line so there's nothing lost in my opinion.
When a worker cannot establish a websocket connection the log line reports the URL of the REST API, and not the websocket endpoint where the connection failed.
Old output:
New output:
Notice the addition of the network error & the final request url where the failre happened.