Behaviors.logMessages implementation by seglo · Pull Request #26238 · akka/akka-core

seglo · 2019-01-12T17:40:39Z

This PR provides a new Akka Typed Behavior that can be decorated with a Receive[T] to log all messages received. This is similar to using LoggingReceive in untyped Akka. See #26226 for some more details.

I have a few questions:

In the Akka logging documentation, it states about LoggingReceive

If you want very detailed logging of user-level messages then wrap your actors’ behaviors with akka.event.LoggingReceive and enable the receive option:

https://doc.akka.io/docs/akka/current/logging.html#auxiliary-logging-options

Should this implementation check that akka.actor.debug.receive = on before logging? I'm not sure this is actually required when using LoggingReceive in Akka untyped.

~~This is a relatively minor addition. Does documentation need to be added?~~ What is the most appropriate place in the user docs to mention this feature?

akka-ci · 2019-01-12T17:41:04Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

patriknw · 2019-01-13T17:00:15Z

Should this implementation check that akka.actor.debug.receive = on before logging?

I think the original reason for having a config property is to be able to have the LoggingReceive in the application's code but only enable it when needed without having to change source code.

That reason is still valid for Typed, but I think it would be more useful to have a way to enable/disable individual logMessages without having to restructure the code. Adding an option Boolean parameter would solve that. An application can use a config value for that value if that is what they want.

I also wonder if we should make it possible to pass in a custom LoggingAdapter, e.g. to be able to give it a custom logger name, see #26200. Then with 3 parameters it's probably better to have a single LogOptions parameter.

Apart from this the implementation and everything in this PR is looking good.

patriknw · 2019-01-13T17:00:46Z

OK TO TEST

akka-ci · 2019-01-13T17:29:37Z

Test PASSed.

seglo

I implemented a LogOptions to group parameters to Behaviors.logMessages based on your feedback. It now logs signals too.

akka-ci · 2019-01-14T19:09:48Z

Test PASSed.

akka-ci · 2019-01-14T20:29:27Z

Test FAILed.

akka-ci · 2019-01-14T21:29:52Z

Test PASSed.

johanandren

Great work, added a few thoughts.

Would be good if the Java API was at least touched from test code, I think compile only would be fine, in akka.actor.typed.javadsl.ActorLoggingTest

johanandren · 2019-01-15T12:13:47Z

+    if (opts.enabled)
+      logger.getOrElse(ctx.asScala.log).log(opts.level, "received message {}", msg)
+    target(ctx, msg)
+  }


The untyped one actually invokes receive and looks at the result to be able to say if it was handled/unhandled in the log message, should we perhaps do that here as well? We could actually even log what the new behavior/state is. WDYT?

I noticed that LoggingReceive uses isDefinedAt to see if a message is handled an encodes that into the message. I wasn't sure how to do that with the Behavior because I don't know how to access the underlying PartialFunction.

We could actually even log what the new behavior/state is.

I'm not sure what you mean by this.

More of an idea than entirely sure we should do this but,
instead of logging before (or in addition to logging before perhaps) we could do something like this:

val newBehavior = target(ctx, msg) val handled = if (newBehavior == Behavior.UnhandledBehavior) "unhandled" else "handled" log.debug("received message {}, was {}") // or even the full state, perhaps behind a separate config flag if (opts.logNewState) log.debug("handled message {}, new behavior {}", msg, newBehavior) newBehavior

It could be useful. I can include it if you like.

I have a question regarding the following line

val handled = if (newBehavior == Behavior.UnhandledBehavior) "unhandled" else "handled"

This is checking if the newBehavior is a Behavior.UnhandledBehavior, but in LoggingReceive the logic is checking whether a message is actually handled by a behavior, which can be done using PartialFunction.isDefinedAt. Is there a way to access the underlying PF of the behavior to do a similar check?

In the typed case we don't know if it is a PF at all, so no, the only way to know that a message was unhandled is to call the behavior and look at the return value. Maybe we could skip that now and look into what would be useful in a follow up PR?

Sounds good to me 👍

Created #26271 to track that

patriknw

looking good, a few nitpicks

seglo

I made a number of updates based on your feedback. I left some comments where I need further clarification. Thanks for your patience reviewing.

akka-ci · 2019-01-15T19:29:21Z

Test FAILed.

akka-ci · 2019-01-15T20:03:49Z

Test FAILed.

seglo · 2019-01-15T20:33:58Z

I'm having trouble reformatting AkkaLoggingTest to make the Jenkins validation build happy. I performed an sbt clean test:compile as the failed build suggests, but it does not reformat the file.

patriknw

LGTM, something very minor...

akka-ci · 2019-01-18T17:15:36Z

Test PASSed.

johanandren

LGTM!

seglo force-pushed the typed-logging-receive branch from e22df82 to a13558b Compare January 12, 2019 18:16

patriknw reviewed Jan 13, 2019

View reviewed changes

Comment thread akka-actor-typed/src/main/scala/akka/actor/typed/internal/InterceptorImpl.scala

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jan 13, 2019

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jan 14, 2019

seglo commented Jan 14, 2019

View reviewed changes

Comment thread akka-actor-typed/src/main/scala/akka/actor/typed/internal/InterceptorImpl.scala Outdated

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jan 14, 2019

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jan 14, 2019

seglo force-pushed the typed-logging-receive branch from b4653dd to b348488 Compare January 14, 2019 20:25

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jan 14, 2019

akka-ci added tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Jan 14, 2019

johanandren reviewed Jan 15, 2019

View reviewed changes

patriknw reviewed Jan 15, 2019

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jan 15, 2019

seglo commented Jan 15, 2019

View reviewed changes

Comment thread akka-actor-typed-tests/src/test/java/akka/actor/typed/javadsl/ActorLoggingTest.java

Comment thread akka-actor-typed-tests/src/test/scala/akka/actor/typed/LogMessagesSpec.scala

akka-ci added the needs-attention Indicates a PR validation failure (set by CI infrastructure) label Jan 15, 2019

akka-ci removed the validating PR is currently being validated by Jenkins label Jan 15, 2019

patriknw approved these changes Jan 16, 2019

View reviewed changes

Comment thread akka-actor-typed/src/main/scala/akka/actor/typed/Logger.scala Outdated

Comment thread akka-actor-typed/src/main/scala/akka/actor/typed/scaladsl/Behaviors.scala Outdated

Behaviors.logMessages implementation akka#26226

fb5d798

seglo force-pushed the typed-logging-receive branch from 29041cd to fb5d798 Compare January 18, 2019 16:44

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jan 18, 2019

johanandren approved these changes Jan 21, 2019

View reviewed changes

johanandren mentioned this pull request Jan 21, 2019

Optional logging of the result behavior in typed #26271

Open

johanandren merged commit 75be2c4 into akka:master Jan 21, 2019

seglo deleted the typed-logging-receive branch January 21, 2019 16:33

Conversation

seglo commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akka-ci commented Jan 12, 2019

Uh oh!

patriknw commented Jan 13, 2019

Uh oh!

Uh oh!

patriknw commented Jan 13, 2019

Uh oh!

akka-ci commented Jan 13, 2019

Uh oh!

seglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akka-ci commented Jan 14, 2019

Uh oh!

akka-ci commented Jan 14, 2019

Uh oh!

akka-ci commented Jan 14, 2019

Uh oh!

johanandren left a comment

Choose a reason for hiding this comment

Uh oh!

johanandren Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

seglo Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

johanandren Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seglo Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

johanandren Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

seglo Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

johanandren Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akka-ci commented Jan 15, 2019

Uh oh!

akka-ci commented Jan 15, 2019

Uh oh!

seglo commented Jan 15, 2019

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akka-ci commented Jan 18, 2019

Uh oh!

johanandren left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

seglo commented Jan 12, 2019 •

edited

Loading

johanandren Jan 16, 2019 •

edited

Loading