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

chore(Orleans.Runtime): Use LoggerMessageGenerator for core logging messages #9316

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Meir017
Copy link

@Meir017 Meir017 commented Feb 6, 2025

part of #9273

Microsoft Reviewers: Open in CodeFlow

_activationCount,
ToString());
}
LogBeforeCollection(number, memBefore, _activationCount, this);

List<ICollectibleGrainContext> list = scanStale ? ScanStale() : ScanAll(ageLimit);
CatalogInstruments.ActivationCollections.Add(1);
Copy link
Author

@Meir017 Meir017 Feb 6, 2025

Choose a reason for hiding this comment

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

@ReubenBond I'm not sure if it's possible to convert the log

if (logger.IsEnabled(LogLevel.Debug)) logger.LogDebug("CollectActivations {Activations}", list.ToStrings(d => d.GrainId.ToString() + d.ActivationId));

to the LoggerMessage syntax, is it possible to have custom formatter for parameters? (to call the .ToStrings method conditionally like you're doing now) - this type of optimization appears in multiple locations

Copy link
Member

Choose a reason for hiding this comment

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

This is a very costly log line... You could convert the LogDebug call but keep the IsEnabled guard to prevent massive unnecessary memory allocation. I'd also consider bumping this to Trace since it's extremely verbose.

Copy link
Author

Choose a reason for hiding this comment

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

@ReubenBond I wonder if we could introduce a utility struct to simplify this

private struct ActivationsLogValue(List<ICollectibleGrainContext> list)
{
    public override string ToString() => list.ToStrings(d => d.GrainId.ToString() + d.ActivationId);
}

and then have the logging method have an argument of type ActivationsLogValue

Copy link
Author

Choose a reason for hiding this comment

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

@ReubenBond thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. Please also switch the level to to Trace if you change this

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.

2 participants