cashapp / misk

Microservice Kontainer
https://cashapp.github.io/misk/
391 stars 166 forks source link

Fix tagged logger concurrent update exception #3270

Closed afkelsall closed 1 month ago

afkelsall commented 1 month ago

The way loggers are accessed, they are most often a static instance shared between threads like so:

class EventConsumer {
  companion object {
      private val logger = this::class.getTaggedLogger()
    }
}

The previous version failed to identify the shared state on the logger where multiple threads consume from event streams, job queues and web actions etc.

This fix changes the way the logger works to reflect our local service TaggedLogger's we've implemented internally at Cash where on each tag("tagKey" to "tagValue123") call the logger copies itself and a new instance with the updated Set<Tag> is passed into the constructor for the next call to the builder or asContext {}. This addresses the concurrent thread problem because each thread copies the logger for itself when building it.

The challenge moving the TaggedLogger implementation into the misk framework where a service will extend the abstract TaggedLogger class is that there needs to be a way for the misk TaggedLogger to get the implementing class to copy itself with new tags. That's where the interface Copyable {} comes in. This ends up making the ServiceTaggedLogger: TaggedLogger() implementation more complex and the generics are more complex, but this is a write once per service class and I think with good documentation and support will be fine.