Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

NHDaly · 2023-08-17T22:22:19Z

This is a follow-up from #1065, fixing a few more cases of the old logging style.

Also adds a test that at least one of those logs does indeed log the exception details in the string, so we know the exception logging is working as expected.

Finally, cleans up a couple unused variables in catch e in 2 places, and updates Base.catch_stack() to Base. current_exceptions(), which is the new, public API for this which is exported from Base. It starts warning about calling the older, deprecated function in 1.9.

quinnj · 2023-08-17T22:53:29Z

I'm seeing

 Test threw exception
  Expression: close(server)
  UndefVarError: current_exceptions not defined
  Stacktrace:
    [1] current_exceptions_to_string()
      @ HTTP.Exceptions ~/work/HTTP.jl/HTTP.jl/src/Exceptions.jl:103
    [2] shutdown(fn::Main.test_server.var"#shutdown_throw#24")
      @ HTTP.Servers ~/work/HTTP.jl/HTTP.jl/src/Servers.jl:196
    [3] foreach(f::typeof(HTTP.Servers.shutdown), itr::Vector{Function})
      @ Base ./abstractarray.jl:2141

in the 1.6 tests

NHDaly · 2023-08-18T03:33:29Z

Aha, yeah, thanks! 👍

test/server.jl

nickrobinson251 · 2023-08-18T08:16:37Z

Some tests are not being run... is that intentional?

I couldn't see an issue about it so i opened one: #1093

quinnj · 2023-08-18T15:23:47Z

Some tests are not being run... is that intentional?

I couldn't see an issue about it so i opened one: #1093

Yes, intentional. Thanks for opening the issue as I forgot about it. The downloads tests were relying on a public server that is no longer serving requests in the same way, so they were just constantly failing, so they were disabled. I pinged @oxinabox about it, but I think it's kind of a pain to dig into and figure out what to do instead.

NHDaly

Friendly ping. Can we review and merge this soon? This was making it harder to debug the AT&T incident we encountered, and I'd like to close the repair item.

test/server.jl

NHDaly

Oh, whoops, i didn't realize the test were failing. I pushed up a change that will hopefully fix it. :)

nickrobinson251 · 2023-09-06T11:02:19Z

Looks like Autobahn Server tests are failing (it seems these tests only run on linux)

Server: Test Failed at /home/runner/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:90
  Expression: v["behavior"] in ("OK", "NON-STRICT", "INFORMATIONAL", "UNIMPLEMENTED")
   Evaluated: "FAILED" in ("OK", "NON-STRICT", "INFORMATIONAL", "UNIMPLEMENTED")

Stacktrace:
 [1] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
 [2] macro expansion
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:90 [inlined]
 [3] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [4] macro expansion
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:80 [inlined]
 [5] macro expansion
   @ /opt/hostedtoolcache/julia/1.9.3/x64/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
 [6] top-level scope
   @ ~/work/HTTP.jl/HTTP.jl/test/websockets/autobahn.jl:22

…tion=`

Co-authored-by: Nick Robinson <npr251@gmail.com>

src/WebSockets.jl

Drvi · 2023-09-18T14:27:18Z

Slightly related to this PR, there is a bad call to current_exceptions_to_string at

HTTP.jl/src/clientlayers/ConnectionRequest.jl

Line 134 in 45b6d92

err = current_exceptions_to_string(e)

NHDaly · 2023-09-18T14:35:23Z

Slightly related to this PR, there is a bad call to current_exceptions_to_string at

HTTP.jl/src/clientlayers/ConnectionRequest.jl

Line 134 in 45b6d92

err = current_exceptions_to_string(e)

Thanks, i've fixed that. ... So we're missing code coverage on that error path, yeah?

codecov-commenter · 2023-09-18T14:44:06Z

Codecov Report

Attention: Patch coverage is 57.14286% with 12 lines in your changes missing coverage. Please review.

Project coverage is 82.67%. Comparing base (bda4ef2) to head (bab328a).
Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
src/clientlayers/ConnectionRequest.jl	0.00%	6 Missing ⚠️
src/clientlayers/StreamRequest.jl	0.00%	2 Missing ⚠️
src/clientlayers/TimeoutRequest.jl	0.00%	2 Missing ⚠️
src/Exceptions.jl	75.00%	1 Missing ⚠️
src/Servers.jl	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1092      +/-   ##
==========================================
+ Coverage   82.49%   82.67%   +0.18%     
==========================================
  Files          32       32              
  Lines        3044     3053       +9     
==========================================
+ Hits         2511     2524      +13     
+ Misses        533      529       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Drvi · 2023-09-18T14:46:45Z

Thanks, i've fixed that. ... So we're missing code coverage on that error path, yeah?

Introducing codecov is the right answer:)

Drvi · 2023-09-18T15:27:19Z

The remaining test failures are also flakes from tests that I introduced very recently #1110. There I test that internal exceptions are not retried by expecting the request to finish way sooner than the total retry delays, but now I think I didn't choose good value for that (max waiting time of 3 seconds was perhaps too ambitious). Sorry for that! I think if we bump the total delay and max waiting time by 10x, it should be less flakey.

NHDaly · 2023-09-18T16:10:01Z

Signoff from @Drvi on slack that we can ignore the Invalidations job failure for now. Thanks!

…tion=` pattern (#1092) * Change logs to use `current_exceptions_to_string()` instead of `exception=` * cleanup unused variable `catch e` * Update Exceptions.jl for older julia * Rename err => msg; fix missing import * Make "Don't retry on internal exceptions" less flakey --------- Co-authored-by: Nick Robinson <npr251@gmail.com> Co-authored-by: Tomáš Drvoštěp <tomas.drvostep@gmail.com>

…tead of `exception=` pattern (#1115) * Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern (#1092) * Change logs to use `current_exceptions_to_string()` instead of `exception=` * cleanup unused variable `catch e` * Update Exceptions.jl for older julia * Rename err => msg; fix missing import * Make "Don't retry on internal exceptions" less flakey --------- Co-authored-by: Nick Robinson <npr251@gmail.com> Co-authored-by: Tomáš Drvoštěp <tomas.drvostep@gmail.com> * Bump release to v1.9.16 for #1092 --------- Co-authored-by: Nick Robinson <npr251@gmail.com> Co-authored-by: Tomáš Drvoštěp <tomas.drvostep@gmail.com>

nickrobinson251 reviewed Aug 18, 2023

View reviewed changes

test/server.jl Outdated Show resolved Hide resolved

nickrobinson251 mentioned this pull request Sep 1, 2023

test/downloads.jl tests not being run #1093

Open

NHDaly commented Sep 6, 2023

View reviewed changes

test/server.jl Outdated Show resolved Hide resolved

NHDaly commented Sep 6, 2023

View reviewed changes

NHDaly and others added 5 commits September 18, 2023 08:20

Change logs to use current_exceptions_to_string() instead of `excep…

9f7cba0

…tion=`

cleanup unused variable catch e

4596219

Update Exceptions.jl for older julia

08f400e

Update test/server.jl

8b8e0b0

Co-authored-by: Nick Robinson <npr251@gmail.com>

Update test/server.jl

45b6d92

NHDaly force-pushed the nhd-logging-patterns branch from 2f950b6 to 45b6d92 Compare September 18, 2023 14:20

Drvi reviewed Sep 18, 2023

View reviewed changes

src/WebSockets.jl Outdated Show resolved Hide resolved

Rename err => msg; fix missing import

c1b4230

Make "Don't retry on internal exceptions" less flakey

bab328a

NHDaly merged commit c83d30f into JuliaWeb:master Sep 18, 2023
10 of 11 checks passed

NHDaly deleted the nhd-logging-patterns branch September 18, 2023 16:10

NHDaly restored the nhd-logging-patterns branch October 2, 2023 15:22

NHDaly deleted the nhd-logging-patterns branch October 2, 2023 15:22

NHDaly added a commit that referenced this pull request Oct 2, 2023

Bump release to v1.9.16 for #1092

7e6abd5

NHDaly mentioned this pull request Oct 2, 2023

Backport #1092: Change logs to use current_exceptions_to_string() instead of exception= pattern #1115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

NHDaly commented Aug 17, 2023

quinnj commented Aug 17, 2023

NHDaly commented Aug 18, 2023

nickrobinson251 commented Aug 18, 2023

quinnj commented Aug 18, 2023

NHDaly left a comment

NHDaly left a comment

nickrobinson251 commented Sep 6, 2023

Drvi commented Sep 18, 2023

NHDaly commented Sep 18, 2023

codecov-commenter commented Sep 18, 2023 •

edited by codecov bot

Loading

Drvi commented Sep 18, 2023

Drvi commented Sep 18, 2023 •

edited

Loading

NHDaly commented Sep 18, 2023

Change logs to use current_exceptions_to_string() instead of exception= pattern #1092

Change logs to use current_exceptions_to_string() instead of exception= pattern #1092

Conversation

NHDaly commented Aug 17, 2023

quinnj commented Aug 17, 2023

NHDaly commented Aug 18, 2023

nickrobinson251 commented Aug 18, 2023

quinnj commented Aug 18, 2023

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly left a comment

Choose a reason for hiding this comment

nickrobinson251 commented Sep 6, 2023

Drvi commented Sep 18, 2023

NHDaly commented Sep 18, 2023

codecov-commenter commented Sep 18, 2023 • edited by codecov bot Loading

Codecov Report

Drvi commented Sep 18, 2023

Drvi commented Sep 18, 2023 • edited Loading

NHDaly commented Sep 18, 2023

Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

Change logs to use `current_exceptions_to_string()` instead of `exception=` pattern #1092

codecov-commenter commented Sep 18, 2023 •

edited by codecov bot

Loading

Drvi commented Sep 18, 2023 •

edited

Loading