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

Custom log site and new black box API #1242

Merged
merged 33 commits into from
Mar 3, 2020
Merged

Custom log site and new black box API #1242

merged 33 commits into from
Mar 3, 2020

Conversation

dmdashenkov
Copy link
Contributor

@dmdashenkov dmdashenkov commented Mar 2, 2020

Log site

Flogger library provides API for changing the header of log messages via LogSite instances. In this PR we add new logging API for all the entities. In a handler method, a user may obtain a preconfigured logger as follows:

class TaskAggregate extends Aggregate<...> implements LoggingEntity {

    @React
    AssigneeRemoved on(UserDeleted event, EventContext ctx) {
        _fine().log("Reacting to an event");
        // ...
    }
}

The resulting log would be similar to:

org.example.TaskAggregate on(UserDeleted, EventContext)
FINE: Reacting to an event

Implementation details

A new interface HandlerLifecycle is introduced. This is a set of callbacks regarding a message handler method invocation.

All the entities which extend AbstractEntity now implement this interface. The framework users may choose to implement it in other message handlers, namely AbstractEventReactor, AbstractEventSubscriber, AbstractCommander, and AbstractCommandHandler subclasses.

The framework users may employ the interface for their own needs, such as tracking the performance of the handler methods, adding extra logging, etc.

Custom actor ID for Black Box Bounded Context

With this PR, we add API for configuring the ID and time zone used to produce actor requests in the Black Box Bounded Context.

In some domains, entities and repositories may rely on the details from the ActorContext, such as the actor ID. One common use case for this is routing commands by the actor ID.

In order to simulate real-life actors in black-box tests, we add the API for changing the actor how posts commands via BlackBoxBoundedContext:

BlackBoxBoundedContext
        .from(builder)
        .withActor(actorId)
        .receivesCommand(..)
        ...

Also, users may change the time zone ID and offset via the in method:

BlackBoxBoundedContext
        .from(builder)
        .in(ZoneIds.of("Europe/Kyiv"), ZoneOffsets.ofHours(2))
        .receivesCommand(..)
        ...

Or do both at the same time via the withActorIn method:

BlackBoxBoundedContext
        .from(builder)
        .withActorIn(actorId, ZoneIds.of("Europe/Kyiv"), ZoneOffsets.ofHours(2))
        .receivesCommand(..)
        ...

Resolves #1226.

@dmdashenkov dmdashenkov self-assigned this Mar 2, 2020
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #1242 into master will not change coverage by %.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1242   +/-   ##
=========================================
  Coverage     91.22%   91.22%           
  Complexity     4763     4763           
=========================================
  Files           620      620           
  Lines         14918    14918           
  Branches        838      838           
=========================================
  Hits          13609    13609           
  Misses         1054     1054           
  Partials        255      255           

@dmdashenkov dmdashenkov marked this pull request as ready for review March 2, 2020 19:03
@dmdashenkov dmdashenkov requested a review from armiol March 2, 2020 19:04
@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.

@alexander-yevsyukov
Copy link
Contributor

alexander-yevsyukov commented Mar 2, 2020

@dmdashenkov, would log tracing improvements work for underscored methods like _debug()?

@dmdashenkov
Copy link
Contributor Author

dmdashenkov commented Mar 3, 2020

@alexander-yevsyukov, no, they wouldn't. These logging configurations only apply to the logs created via the log().at(...) construct. The method is deliberately named log() and not logger() to avoid a naming clash if a user wants to use Logging in their entity.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@dmdashenkov LGTM with some minor comments.

Also, as discussed vocally, we probably need to rename AbstractEntity.log() method into something, which would make it clear, why its invocation result differs from _debug(), _info() and others.


@CheckReturnValue
@ParametersAreNonnullByDefault
package io.spine.server.log.given;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document this package.

}

message ReturnBook {
LibraryCardId card = 1 [(required) = true, (validate) = true];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we intend to validate LibraryCardId.reader? If so, then LibraryCardId should also say the reader is required.

@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL again.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

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

@dmdashenkov as long as you change the PR description, feel free to merge.

@dmdashenkov dmdashenkov merged commit 2a90f60 into master Mar 3, 2020
@dmdashenkov dmdashenkov deleted the custom-log-site branch March 3, 2020 16:16
@armiol armiol mentioned this pull request Mar 8, 2020
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.

Add an ability to set the ActorId in BlackBoxBoundedContext
3 participants