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.

Sign up for GitHub

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

Jump to bottom

Log whether the localization resource is found or not #55897

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

Log whether the localization resource is found or not #55897

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67c16cd
Log whether the localization resource is found or not
hishamco May 27, 2024
b8840b9
Fix unit test
hishamco May 27, 2024
f5b8fe5
Update src/Localization/Localization/src/ResourceManagerStringLocaliz…
hishamco Jun 1, 2024
a47ed95
Apply suggestions from code review
hishamco Jun 12, 2024
fe80dc3
Update src/Localization/Localization/src/ResourceManagerStringLocaliz…
hishamco Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/Localization/Localization/src/ResourceManagerStringLocalizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,16 +166,32 @@ protected IEnumerable<LocalizedString> GetAllStrings(bool includeParentCultures,

if (_missingManifestCache.ContainsKey(cacheKey))
{
Log.ResourceNotFound(_logger, name);

return null;
}

try
{
return _resourceManager.GetString(name, culture);
var value = _resourceManager.GetString(name, culture);

if (name == value)
{
Log.ResourceFound(_logger, name);
}
else
{
Log.ResourceNotFound(_logger, name);
}
hishamco marked this conversation as resolved.
Show resolved Hide resolved

return value;
}
catch (MissingManifestResourceException)
{
_missingManifestCache.TryAdd(cacheKey, null);

Log.ResourceNotFound(_logger, name);
Copy link
Member

Choose a reason for hiding this comment

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

Should we provide a different log for a missing manifest vs a non-localized resource where the name == value?

@DamianEdwards do you have an opinion 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.

No real opinion, I'd defer to folks who've been using it for real to inform us to how useful that would be.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO we should log for both

Copy link
Member

@halter73 halter73 May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree we should log in both cases. I was asking if there should be a different log message for the two places this PR currently calls Log.ResourceNotFound since the resource was not found for slightly different reasons, but I'm fine with using the same message whenever a resource is not found for any reason like the PR does currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we could change the log level for missing manifest to trace so in debug the missing resources will be logged which are useful in almost cases

Copy link
Member

Choose a reason for hiding this comment

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

I'd just leave them exactly the same for now. If we were going to change the log level, we'd want to give it a unique event id and log message. Changing the level of a log but nothing else would be confusing.

hishamco marked this conversation as resolved.
Show resolved Hide resolved

return null;
}
}
Expand Down Expand Up @@ -221,5 +237,11 @@ private static partial class Log
{
[LoggerMessage(1, LogLevel.Debug, $"{nameof(ResourceManagerStringLocalizer)} searched for '{{Key}}' in '{{LocationSearched}}' with culture '{{Culture}}'.", EventName = "SearchedLocation")]
public static partial void SearchedLocation(ILogger logger, string key, string locationSearched, CultureInfo culture);

[LoggerMessage(2, LogLevel.Debug, $"The resource with key '{{Key}}' is not found.", EventName = "ResourceNotFound")]
hishamco marked this conversation as resolved.
Show resolved Hide resolved
public static partial void ResourceNotFound(ILogger logger, string key);

[LoggerMessage(3, LogLevel.Debug, $"The resource with key '{{Key}}' is found.", EventName = "ResourceFound")]
Copy link
Member

@halter73 halter73 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
[LoggerMessage(3, LogLevel.Debug, $"The resource with key '{{Key}}' is found.", EventName = "ResourceFound")]

Edit: I'd rather we remove this log altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, you will the non found resources?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand your question, but I'm asking that we only add the "ResourceNotFound" log not the "ResourceFound" log.

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot my above comment seems I miss some words during the writing :)

public static partial void ResourceFound(ILogger logger, string key);
hishamco marked this conversation as resolved.
Show resolved Hide resolved
}
}
6 changes: 4 additions & 2 deletions ...zation/test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,10 @@ public void GetString_LogsLocationSearched()
var value = localizer["a key!"];

// Assert
var write = Assert.Single(Sink.Writes);
Assert.Equal("ResourceManagerStringLocalizer searched for 'a key!' in 'Resources.TestResource' with culture 'en-US'.", write.State.ToString());
var write = Sink.Writes;
Assert.Equal(2, write.Count);
Assert.Equal("ResourceManagerStringLocalizer searched for 'a key!' in 'Resources.TestResource' with culture 'en-US'.", write.ElementAt(0).State.ToString());
Assert.Equal("The resource with key 'a key!' is found.", write.ElementAt(1).State.ToString());
}

[Theory]
Expand Down
Loading