Skip to content
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

AsyncLogger: enqueue logging operation instead of a log message #461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Jun 20, 2022

My attempt to fix #460.

This approach fixes the underlying issue, but it affects the performance as well.

1) AsyncLoggerBenchmark

Original benchmarks of async logger are a little bit of cheating. Since they are basically measuring the queue.offer operation.

Before (master branch):

Benchmark                             Mode  Cnt    Score     Error  Units
AsyncLoggerBenchmark.msg              avgt   25  1241.937 ±  276.487  ns/op
AsyncLoggerBenchmark.msgAndCtx        avgt   25  1274.021 ±   82.916  ns/op
AsyncLoggerBenchmark.msgCtxThrowable  avgt   25  2021.246 ± 1326.558  ns/op

After (fix branch):

Benchmark                             Mode  Cnt    Score     Error  Units
AsyncLoggerBenchmark.msg              avgt   25  699.431 ±  73.839  ns/op
AsyncLoggerBenchmark.msgAndCtx        avgt   25  716.672 ± 105.136  ns/op
AsyncLoggerBenchmark.msgCtxThrowable  avgt   25  926.306 ± 568.412  ns/op

2) Measure the cost of the drain operation

I decided to measure the cost of the drain operation as well. I tweaked AsyncLoggerBenchmark for this purpose:

val (asyncLogger: Logger[IO], cancelToken: IO[Unit]) =
  fileLogger[IO](fileName).withAsync(
+   timeWindow = Int.MaxValue.seconds, // to prevent background flushes
    maxBufferSize = Some(1000000)
  ).allocated.unsafeRunSync()
    
@Benchmark
@OperationsPerInvocation(1000)
def msg(): Unit = 
  (1 to 1000).toList.traverse(_ => asyncLogger.info(message))
+   .flatMap(_ => asyncLogger.drain) 
    .unsafeRunSync()

Before (master branch):

Benchmark                             Mode  Cnt     Score    Error  Units
AsyncLoggerBenchmark.msg              avgt   25  1021.347 ± 32.193  ns/op
AsyncLoggerBenchmark.msgAndCtx        avgt   25  1013.955 ± 68.780  ns/op
AsyncLoggerBenchmark.msgCtxThrowable  avgt   25  4804.478 ± 99.426  ns/op

After (fix branch):

Benchmark                             Mode  Cnt     Score     Error  Units
AsyncLoggerBenchmark.msg              avgt   25  3483.380 ± 125.855  ns/op
AsyncLoggerBenchmark.msgAndCtx        avgt   25  3478.960 ±  95.501  ns/op
AsyncLoggerBenchmark.msgCtxThrowable  avgt   25  7026.014 ± 107.790  ns/op

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (db4444f) to head (9cc6cb5).

Files Patch % Lines
...e/src/main/scala/io/odin/loggers/AsyncLogger.scala 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
+ Coverage   89.16%   89.21%   +0.04%     
==========================================
  Files          35       35              
  Lines         517      519       +2     
  Branches       12        8       -4     
==========================================
+ Hits          461      463       +2     
  Misses         56       56              
Flag Coverage Δ
unittests 89.21% <91.66%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@iRevive iRevive changed the title AsyncLogger: enqueue logging effect instead of a log message AsyncLogger: enqueue logging operation instead of a log message Jun 20, 2022
@ybasket ybasket mentioned this pull request Aug 6, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withMinimalLevel does not work properly with AsyncLogger
1 participant