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

Behaviors.logMessages implementation #26238

Merged
merged 1 commit into from Jan 21, 2019
Merged

Conversation

@seglo
Copy link
Member

@seglo seglo commented Jan 12, 2019

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:

  1. 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.

  1. 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
Copy link

@akka-ci akka-ci commented Jan 12, 2019

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!

@seglo seglo force-pushed the typed-logging-receive branch from e22df82 to a13558b Jan 12, 2019
@patriknw
Copy link
Member

@patriknw patriknw commented Jan 13, 2019

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
Copy link
Member

@patriknw patriknw commented Jan 13, 2019

OK TO TEST

@akka-ci
Copy link

@akka-ci akka-ci commented Jan 13, 2019

Test PASSed.

@akka-ci akka-ci added validating and removed tested labels Jan 14, 2019
Copy link
Member Author

@seglo seglo left a comment

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

@akka-ci akka-ci added tested and removed validating labels Jan 14, 2019
@akka-ci
Copy link

@akka-ci akka-ci commented Jan 14, 2019

Test PASSed.

@akka-ci akka-ci added validating and removed tested labels Jan 14, 2019
@seglo seglo force-pushed the typed-logging-receive branch from b4653dd to b348488 Jan 14, 2019
@akka-ci
Copy link

@akka-ci akka-ci commented Jan 14, 2019

Test FAILed.

@akka-ci
Copy link

@akka-ci akka-ci commented Jan 14, 2019

Test PASSed.

Copy link
Member

@johanandren johanandren left a comment

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

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

@johanandren johanandren Jan 15, 2019

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?

Copy link
Member Author

@seglo seglo Jan 15, 2019

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.

Copy link
Member

@johanandren johanandren Jan 16, 2019

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

Copy link
Member Author

@seglo seglo Jan 18, 2019

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?

Copy link
Member

@johanandren johanandren Jan 19, 2019

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?

Copy link
Member Author

@seglo seglo Jan 19, 2019

Sounds good to me 👍

Copy link
Member

@johanandren johanandren Jan 21, 2019

Created #26271 to track that

Copy link
Member

@patriknw patriknw left a comment

looking good, a few nitpicks

@akka-ci akka-ci added validating and removed tested labels Jan 15, 2019
Copy link
Member Author

@seglo seglo left a comment

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
Copy link

@akka-ci akka-ci commented Jan 15, 2019

Test FAILed.

@akka-ci
Copy link

@akka-ci akka-ci commented Jan 15, 2019

Test FAILed.

@seglo
Copy link
Member Author

@seglo seglo commented Jan 15, 2019

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.

Copy link
Member

@patriknw patriknw left a comment

LGTM, something very minor...

@akka-ci
Copy link

@akka-ci akka-ci commented Jan 18, 2019

Test PASSed.

Copy link
Member

@johanandren johanandren left a comment

LGTM!

@johanandren johanandren merged commit 75be2c4 into akka:master Jan 21, 2019
3 checks passed
@seglo seglo deleted the typed-logging-receive branch Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants