Server doesn't print unexpected error logs (#2491)

[cheikhsimsol]

fixes #2491 /claim #2491

The approach here is to update the fromCause

[Update] Managed to add some form of testing.

[CLAassistant]

All committers have signed the CLA.

[algora-pbc[bot]]

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ca46f06) 64.76% compared to head (d8057b8) 64.75%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2602      +/-   ##
==========================================
- Coverage   64.76%   64.75%   -0.01%     
==========================================
  Files         140      140              
  Lines        8488     8493       +5     
  Branches     1574     1553      -21     
==========================================
+ Hits         5497     5500       +3     
- Misses       2991     2993       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[vigoo]

I think this should be done through ZIO's logging instead of directly printing to the stderr, so it gets integrated with all the logs of a server.

[cheikhsimsol]

Ok, I'll update it to use it.

[cheikhsimsol]

To confirm @vigoo, you're referring to zio.Console or zio.logging?

[vigoo]

No, to ZIO.logError etc (https://zio.dev/reference/observability/logging/)

[cheikhsimsol]

@vigoo I've updated it to use logError, please check and confirm.

[vigoo]

Sorry previously I did not properly check where the logging is added and only reacted to how is it logging.

I would not add any logging into constructors of a Response - those are just data structures and creating a response should not do any side effect. .

Beside that there are two other problems as well:

• Using Runtime.default is not correct, we need to use the runtime the server layer was created with. That way the logging effect will use the configured loggers.
• There is a printStackTrace as well which should also be logged through ZIO logging.

Basically both the Throwable and the Cause case should be logged with ZIO.logErrorCause using the runtime the server layer was created with.

I will take a look at the codebase and recommend a better place for this in my next comment.

[cheikhsimsol]

@vigoo I requested the review by mistake, no need to check.

[vigoo]

An additional question could be whether we always want to do this logging or not. I'm not sure about it - maybe there could be a fiber ref to configure it? @jdegoes what do you think?

Or maybe it could be always logged in the invalid request case (or configured via server config) and there could be a separate sandboxLogged or similar variant that does the logging.

[cheikhsimsol]

With one approach, I tried to throw an exception, although it logs the error, all of the tests break. I updates the test itself to catch and read the exception. However, I'm using printStackTrace to get around needing the runtime.. I'm new to Scala so unsure if this will be ideal.

[jdegoes]

@vigoo If there will be performance cost, it's important that it be optional.

[987Nabil]

@cheikhsimsol can you update your PR please?

[cheikhsimsol]

@cheikhsimsol can you update your PR please?

Please give me some additional time, I'll try to re-visit this during the weekend.

[jdegoes]

@cheikhsimsol Please re-open if you get a chance to fix this up!