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.

Sign up for GitHub

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

Add some further logging #358

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

Add some further logging #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions core/src/main/scala/jms4s/jms/JmsContext.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ class JmsContext[F[_]: Async: Logger](private val context: JMSContext) {
def send(destinationName: DestinationName, message: JmsMessage): F[Unit] =
for {
destination <- createDestination(destinationName)
_ <- Logger[F].debug(s"Creating producer")
p <- Sync[F].blocking(context.createProducer())
_ <- Logger[F].debug(s"Sending message $message to destination: $destinationName")
_ <- Sync[F].blocking(p.send(destination.wrapped, message.wrapped))
_ <- Logger[F].debug(s"Sent message $message to destination: $destinationName")
} yield ()

def send(destinationName: DestinationName, message: JmsMessage, delay: FiniteDuration): F[Unit] =
Expand Down Expand Up @@ -86,10 +89,12 @@ class JmsContext[F[_]: Async: Logger](private val context: JMSContext) {
def rollback: F[Unit] = Sync[F].blocking(context.rollback())

private def createQueue(queue: QueueName): F[JmsQueue] =
Sync[F].blocking(context.createQueue(queue.value)).map(new JmsQueue(_))
Logger[F].debug(s"Creating Queue $queue") *>
Sync[F].blocking(context.createQueue(queue.value)).map(new JmsQueue(_))

private def createTopic(topicName: TopicName): F[JmsTopic] =
Sync[F].blocking(context.createTopic(topicName.value)).map(new JmsTopic(_))
Logger[F].debug(s"Creating Topic $topicName") *>
Sync[F].blocking(context.createTopic(topicName.value)).map(new JmsTopic(_))

def createDestination(destination: DestinationName): F[JmsDestination] = destination match {
case q: QueueName => createQueue(q).widen[JmsDestination]
Expand Down
11 changes: 8 additions & 3 deletions core/src/main/scala/jms4s/jms/JmsMessageConsumer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ import cats.effect.{ Async, Spawn, Sync }
import cats.syntax.all._

import scala.concurrent.duration.FiniteDuration
import org.typelevel.log4cats.Logger

import javax.jms.JMSConsumer

class JmsMessageConsumer[F[_]: Async] private[jms4s] (
class JmsMessageConsumer[F[_]: Async: Logger] private[jms4s] (
private[jms4s] val wrapped: JMSConsumer,
private[jms4s] val pollingInterval: FiniteDuration
) {
Expand All @@ -37,8 +38,12 @@ class JmsMessageConsumer[F[_]: Async] private[jms4s] (
for {
recOpt <- Sync[F].blocking(Option(wrapped.receiveNoWait()))
rec <- recOpt match {
case Some(message) => Sync[F].pure(new JmsMessage(message))
case None => Spawn[F].cede >> Async[F].sleep(pollingInterval) >> receiveJmsMessage
case Some(message) =>
Sync[F].pure(new JmsMessage(message)) <* Logger[F].debug(s"Received message: $message")
case None =>
Spawn[F].cede >> Async[F].sleep(pollingInterval) >> Logger[F].trace(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only doubt will be this log.
I mean, if you are not receving nothing, you are polling.. and the pollingInterval is a static configuration. So right now i cant see where this can be helpful, can you give me an example?
also, yeah it's in trace but this could be a very hot path that could lead to a large amounts of logs and noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@faustin0 Sure. Several times whilst trying to make changes to jms4s I have come across the situation where my tests just hang. Retrieving the jstack showed nothing useful, after debugging line by line, I came to realise that this is because the test is trying to read something from either an empty or wrong queue, and so the test will never complete. After fixing the root cause issue, all was fine, however it was impossible to spot with a standard jstack dump due to the nature of cats-effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, got it. However, I don't think this is the right place to put these logs:

  • if you want to log every consumed message, I will leave this choice to the user, and the most appropriate place is in the handle method exposed by the various consumers, where you have the same message you would log here.
  • the logged message will be inconsistent between the different implementations.
  • logging all the body(that can be large) can lead to some issues and i prefer leave this choiche to the client instead of the library.
  • as for the "polling" logs, yeah maybe they are useful when debugging the libray, but for the users of the library this dont give any useful information so i would remove it and just place a log during development if needed.

s"JmsMessageConsumer#receiveJmsMessage slept: $pollingInterval"
) >> receiveJmsMessage
}
} yield rec
}