Log diagnostic info about search for localization resources #331

ryanbrandenburg · 2017-02-18T00:52:40Z

Fixes #258.

hishamco · 2017-02-19T21:41:09Z

src/Microsoft.Extensions.Localization.Abstractions/LocalizedString.cs

@@ -64,6 +77,11 @@ public LocalizedString(string name, string value, bool resourceNotFound)
        public bool ResourceNotFound { get; }

        /// <summary>
+        /// The location which was searched for a locatization value.
+        /// </summary>
+        public string LocationSearched { get; }


I'm still wonder why we have this per LocalizedString while the searched location is common for all of them

@hishamco - isn't that the behavior of the ResourceManagerStringLocalizer and not necessarily a trait of this type? Plus the intent of making this change was to help diagnosing given that you have a localized string.

@ryanbrandenburg - SearchedLocation?

I didn't said its the behavior of ResourceManagerStringLocalizer 😃 , even the intent of making this change was to help diagnosing given that you have a localized string. I'm still disagree to add the LocationSearched or SearchedLocation to the LocalizedString itself, if we think deeply for a minute the ResourceNotFound is suitable here, because we may find the resource or might not, but the SearchedLocation is something generic for all the resources

I will try to create a branch in my localization repo to prove the concept

pranavkm · 2017-02-22T15:19:34Z

src/Microsoft.Extensions.Localization.Abstractions/LocalizedString.cs

@@ -64,6 +77,11 @@ public LocalizedString(string name, string value, bool resourceNotFound)
        public bool ResourceNotFound { get; }

        /// <summary>
+        /// The location which was searched for a locatization value.
+        /// </summary>
+        public string LocationSearched { get; }


@hishamco - isn't that the behavior of the ResourceManagerStringLocalizer and not necessarily a trait of this type? Plus the intent of making this change was to help diagnosing given that you have a localized string.

pranavkm · 2017-02-22T15:20:12Z

src/Microsoft.Extensions.Localization.Abstractions/LocalizedString.cs

@@ -64,6 +77,11 @@ public LocalizedString(string name, string value, bool resourceNotFound)
        public bool ResourceNotFound { get; }

        /// <summary>
+        /// The location which was searched for a locatization value.
+        /// </summary>
+        public string LocationSearched { get; }


@ryanbrandenburg - SearchedLocation?

pranavkm · 2017-02-22T15:20:22Z

...Microsoft.Extensions.Localization/Internal/ResourceManagerStringLocalizerLoggerExtensions.cs

+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. 
+
+using Microsoft.Extensions.Logging;
+using System;


pranavkm · 2017-02-22T15:20:42Z

src/Microsoft.Extensions.Localization/Microsoft.Extensions.Localization.csproj

@@ -15,6 +15,7 @@
    <ProjectReference Include="..\Microsoft.Extensions.Localization.Abstractions\Microsoft.Extensions.Localization.Abstractions.csproj" />
    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="1.2.0-*" />
    <PackageReference Include="Microsoft.Extensions.Options" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.Extensions.Logging" Version="1.2.0-*" />


Logging.Abstractions

pranavkm · 2017-02-22T15:21:46Z

src/Microsoft.Extensions.Localization/ResourceManagerStringLocalizer.cs

            _resourceStringProvider = resourceStringProvider;
            _resourceManager = resourceManager;
            _resourceBaseName = baseName;
            _resourceNamesCache = resourceNamesCache;
+            _loggerFactory = loggerFactory;


Don't need this field.

pranavkm · 2017-02-22T15:31:28Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerFactoryTest.cs

@@ -75,7 +80,8 @@ public void Create_FromType_ReturnsCachedResultForSameType()
            var locOptions = new LocalizationOptions();
            var options = new Mock<IOptions<LocalizationOptions>>();
            options.Setup(o => o.Value).Returns(locOptions);
-            var factory = new ResourceManagerStringLocalizerFactory(localizationOptions: options.Object);
+            var loggerFactory = GetLoggerFactory();


Use NullLoggerFactory.Instance

pranavkm · 2017-02-22T15:31:45Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

@@ -1,13 +1,14 @@
 // Copyright (c) .NET Foundation. All rights reserved. 
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using Microsoft.Extensions.Localization.Internal;


sort. System namespaces first.

pranavkm · 2017-02-22T15:33:36Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

+            }
+        }
+
+        public class TestLoggerFactory : ILoggerFactory


Use Microsoft.Extensions.Logging.Testing.TestLoggerFactory.

pranavkm · 2017-02-22T15:34:50Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

@@ -77,6 +84,56 @@ public void EnumeratorCacheIsScopedByAssembly()
            Assert.Equal(expectedCallCount, resourceAssembly2.GetManifestResourceStreamCallCount);
        }

+        [Fact]
+        public void ResourceManagerStringLocalizer_GetString_LocationSearchedPopulates()


GetString_PopulatesSearchedLocationOnLocalizedString

pranavkm · 2017-02-22T15:35:34Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

+                loggerFactory);
+
+            // Act
+            // We have to access the result so it evaluates.


This comment doesn't really make sense.

ryanbrandenburg · 2017-02-22T21:01:56Z

🆙📅

pranavkm

make sure you remove the .Testing reference from the src project. We definitely don't want that.

pranavkm · 2017-02-22T21:39:44Z

src/Microsoft.Extensions.Localization/Microsoft.Extensions.Localization.csproj

@@ -15,6 +15,8 @@
    <ProjectReference Include="..\Microsoft.Extensions.Localization.Abstractions\Microsoft.Extensions.Localization.Abstractions.csproj" />
    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="1.2.0-*" />
    <PackageReference Include="Microsoft.Extensions.Options" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="1.2.0-*" />
+    <PackageReference Include="Microsoft.AspNetCore.Testing" Version="1.2.0-*" />


Wrong project?

pranavkm · 2017-02-22T21:42:15Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

@@ -162,6 +226,17 @@ private static int GetCultureInfoDepth(CultureInfo culture)
            return result;
        }

+
+        private TestSink _sink = new TestSink();


These need to go at the top of the class. Also

private readonly TestSink _sink = new TestSink(); private readonly ILogger Logger { get; } = ew TestLoggerFactory(_sink, true).CreateLogger<ResourceManagerStringLocalizer>();

^ does the second line work?

Unfortunatly no, you get "A field initializer cannot reference the non-static field, method or property '_sink'".

pranavkm · 2017-02-22T21:44:11Z

...Microsoft.Extensions.Localization/Internal/ResourceManagerStringLocalizerLoggerExtensions.cs

+            _searchedLocation = LoggerMessage.Define<string, string, CultureInfo>(
+                LogLevel.Debug,
+                1,
+                $"{nameof(ResourceManagerStringLocalizer)} searched for '{{Key}}' in '{{LocationSearched}}' with '{{Culture}}'.");


with culture `{{Culture}}'

hishamco · 2017-02-23T02:32:22Z

test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerTest.cs

+
+            // Assert
+            Assert.Equal(1, Sink.Writes.Count);
+            Assert.Equal("ResourceManagerStringLocalizer searched for 'a key!' in 'Resources.TestResource' with 'en-US'.", Sink.Writes.First().State.ToString());


with culture 'en-US'.

hishamco · 2017-02-23T03:43:52Z

src/Microsoft.Extensions.Localization.Abstractions/LocalizedString.cs

+        /// <param name="name">The name of the string in the resource it was loaded from.</param>
+        /// <param name="value">The actual string.</param>
+        /// <param name="resourceNotFound">Whether the string was not found in a resource. Set this to <c>true</c> to indicate an alternate string value was used.</param>
+        /// <param name="locationSearched">The location which was searched for a locatization value.</param>


typo in locatization

hishamco · 2017-02-23T03:45:02Z

src/Microsoft.Extensions.Localization.Abstractions/LocalizedString.cs

@@ -64,6 +77,11 @@ public LocalizedString(string name, string value, bool resourceNotFound)
        public bool ResourceNotFound { get; }

        /// <summary>
+        /// The location which was searched for a locatization value.


Typo is hard during copy & past 😄

hishamco · 2017-02-23T03:46:30Z

src/Microsoft.Extensions.Localization/ResourceManagerStringLocalizerFactory.cs

@@ -7,6 +7,7 @@
 using System.Reflection;
 using System.Resources;
 using Microsoft.Extensions.Options;
+using Microsoft.Extensions.Logging;


Should go before

using Microsoft.Extensions.Options;

dnfclas added the cla-already-signed label Feb 18, 2017

hishamco reviewed Feb 19, 2017

View reviewed changes

ryanbrandenburg requested a review from pranavkm February 21, 2017 18:07

pranavkm suggested changes Feb 22, 2017

View reviewed changes

ryanbrandenburg force-pushed the rybrande/DiagnosticInfo branch from 4fe6a2a to c6e4a69 Compare February 22, 2017 21:01

pranavkm approved these changes Feb 22, 2017

View reviewed changes

hishamco reviewed Feb 23, 2017

View reviewed changes

Add location to LocalizedString and add logging

7a8d74f

ryanbrandenburg force-pushed the rybrande/DiagnosticInfo branch from a256177 to 7a8d74f Compare February 24, 2017 18:53

ryanbrandenburg merged commit 39aa943 into dev Feb 24, 2017

hishamco mentioned this pull request Mar 4, 2017

Log all searched locations for non existance resources #336

Closed

JunTaoLuo deleted the rybrande/DiagnosticInfo branch May 23, 2017 19:11

Log diagnostic info about search for localization resources #331

Log diagnostic info about search for localization resources #331

Uh oh!

Conversation

ryanbrandenburg commented Feb 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanbrandenburg commented Feb 22, 2017

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!