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

Made akka.cluster.sharding.verbose-debug-logging accept a LogLevel #7118

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Mar 12, 2024

Changes

close #7097

For users with a large number of sharded entities, the akka.cluster.sharding.verbose-debug-logging HOCON setting now accepts a LogLevel argument. It also still supports the same boolean values as before too.

However, if you were to pass in the following HOCON:

akka.cluster.sharding.verbose-debug-logging = INFO

All of the internal ShardCoordinator logs could now be captured at a LogLevel.Info level, so there wouldn't be any need to turn LogLevel.Debug on globally to capture these logs.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Detailed my changes

@@ -28,7 +28,7 @@ public class ClusterShardingLeavingSpecConfig : MultiNodeClusterShardingConfig

public ClusterShardingLeavingSpecConfig(StateStoreMode mode)
: base(mode: mode, loglevel: "DEBUG", additionalConfig: @"
akka.cluster.sharding.verbose-debug-logging = on
akka.cluster.sharding.verbose-debug-logging = INFO
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test case to ensure that this actually works with INFO logging here. Going to inspect the output once this runs.

@@ -29,7 +30,7 @@ internal sealed class DDataShardCoordinator : ActorBase, IWithTimers, IWithUnbou

private sealed class RememberEntitiesStoreStopped
{
public static RememberEntitiesStoreStopped Instance = new();
public static readonly RememberEntitiesStoreStopped Instance = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor fix unrelated to this PR, but noticed it while I was working.

@@ -77,7 +78,7 @@ private RememberEntitiesLoadTimeout()

private readonly IActorRef _replicator;
private readonly ShardCoordinator _baseImpl;
private bool VerboseDebug => _baseImpl.VerboseDebug;
private LogLevel? VerboseDebug => _baseImpl.VerboseDebug;
Copy link
Member Author

Choose a reason for hiding this comment

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

Change from a bool to a LogLevel? - these are all private APIs so there's no user-facing impact here other than the logging output.

@@ -108,7 +109,7 @@ private RememberEntitiesLoadTimeout()
{
_replicator = replicator;
var log = Context.GetLogger();
var verboseDebug = Context.System.Settings.Config.GetBoolean("akka.cluster.sharding.verbose-debug-logging");
var verboseDebug = ParseVerboseLogSettings(Context.System.Settings.Config);
Copy link
Member Author

Choose a reason for hiding this comment

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

Added a utility method to parse these configuration settings into LogLevel? since we have to make this call from both the DDataShardCoordinator and the PersistentShardCoordinator.

if (VerboseDebug)
Log.Debug("{0}: Received initial coordinator state [{1}]", TypeName, existingState);
else
if (VerboseDebug.HasValue)
Copy link
Member Author

Choose a reason for hiding this comment

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

The way I handled this type of case, which came up in a few areas:

  • In the past we use to have two different Debug calls - one with more data when VerboseDebugging was enabled and then a regular one that was used with Log.IsDebugEnabled was true.
  • All of these calls have been converted to include the first call to use whatever LogLevel is specified by VerboseDebugging in the event that VerboseDebugging is non-null.
  • If VerboseDebugging is null and Log.IsDebugEnabled, we record the "lighter" information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Internal utility class for parsing the LogLevel?

# in production systems.
# in production systems.
# Values can be `off`, `on`, or an `akka.loglevel` value such as `INFO`.
# When set to `on` these logs will be logged using `LogLevel.Debug`.
Copy link
Member Author

Choose a reason for hiding this comment

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

New comments describing the underlying HOCON changes. The default value of the verbose-debug-logging setting has been left intact.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

This is a very weird design decision, but I guess it is needed. I only have one change request.

Comment on lines 30 to 31
"off" => null,
"on" => LogLevel.DebugLevel,
Copy link
Contributor

Choose a reason for hiding this comment

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

To conform to HOCON standard, this will also need to check for "true" and "false" value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - will do.

@Aaronontheweb
Copy link
Member Author

This is a very weird design decision, but I guess it is needed. I only have one change request.

I think this comes down to that our log sources aren't easily filterable on the back-end, so it has to happen on the front-end. This change is also pretty easy to reverse if there were an issue with it.

@Aaronontheweb
Copy link
Member Author

I think this comes down to that our log sources aren't easily filterable on the back-end

I actually have some ideas on how that can be addressed too, in the grand scheme of things, but that'd be a 1.6 or 2.0 feature.

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) March 12, 2024 16:13
auto-merge was automatically disabled March 12, 2024 17:20

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable debug logging for ShardCoordinator/ShardRegion/Shard actors
2 participants