feat: Adds GoogleLogger to Diagnostics.Common #6657

amanda-tarafa · 2021-06-24T01:03:53Z

As always, one commit at a time, although I'll rebase everything into one before merging.

We can make more AspNetCore* types obsolete, and I also want to improve docs, but I think this is now good for a beta release.

amanda-tarafa · 2021-06-24T01:11:25Z

(build error is because of the language version missmatch, I'll figure it out, but this is all green on local so it's ready for review).

jskeet

Okay, I'm glad you've got a handle on this... it looks okay to me, but it's slightly hard to follow what's going on, despite your evident care in creating the commits separately... it's just inherently complex.

Looks fine as far as I can see... if you're confident in it, let's merge (once the language issues have been fixed) and release a beta.

jskeet · 2021-06-24T09:44:57Z

...stics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.IntegrationTests/Logging/LoggingTest.cs

@@ -545,11 +547,11 @@ public async Task Logging_Trace_External_OneEntry(TestServer server)
                LogEntry entry = Assert.Single(results);

                // And the resource name of the trace associated to it contains the external trace id.
-                Assert.Contains(TestEnvironment.GetTestProjectId(), entry.Trace);
-                Assert.Contains("external_trace_id", entry.Trace);
+                      Assert.Contains(TestEnvironment.GetTestProjectId(), entry.Trace);


Nit: indentation

jskeet · 2021-06-24T09:46:35Z

...Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Tests/Logging/GoogleLoggerTest.cs

            string fullTraceName = TraceTarget.ForProject(ProjectId).GetFullTraceName(traceId);

            Predicate<IEnumerable<LogEntry>> matcher = logEntries =>
            {
                LogEntry entry = logEntries.Single();
                return entry.LogName == new LogName(ProjectId, BaseLogName).ToString() &&
                    entry.Trace == fullTraceName &&
-                    entry.SpanId == spanId;
+                    entry.SpanId == $"{spanId:x16}";


It's unclear to me why this had to change. Is it because we're not requiring the external span IDs to be 64-bit integers?

Yes, the now obsolete TraceContextForLogEntry has a string for SpanId, and docs saying that it should be in base 16. But now we use the new ITraceContext where span ID is a ulong, and for setting it in the entry we are the ones who have to convert it to base 16.

jskeet · 2021-06-24T09:47:09Z

...iagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore/Logging/TraceContextForLogEntry.cs

@@ -33,6 +33,7 @@ namespace Google.Cloud.Diagnostics.AspNetCore
    /// These values can be attached to a log entry to establish the
    /// relation of it and a trace.
    /// </summary>
+    [Obsolete("Use Common.ITraceContext instead.")]


Maybe fully-qualify this name?

jskeet · 2021-06-24T09:47:39Z

...Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Tests/Logging/GoogleLoggerTest.cs

@@ -67,10 +67,12 @@ private string Formatter(string message, Exception ex)
            Common.LoggerOptions options = Common.LoggerOptions.CreateWithServiceContext(
                logLevel, logName, labels, monitoredResource, retryOptions: retryOptions, serviceName: serviceName, version: version);
 #pragma warning disable CS0618 // Type or member is obsolete


Do we still need this pragma?

Yes, this is the one that is used to keep backward compatibility.

jskeet · 2021-06-24T09:48:40Z

...Cloud.Diagnostics.Common/Google.Cloud.Diagnostics.Common/KeyValuePairEnumerableExtensions.cs

@@ -23,7 +23,7 @@ namespace Google.Cloud.Diagnostics.Common
    /// Extension methods for converting KeyValuePair Enumerables to several types,
    /// including Protobuf well known types.
    /// </summary>
-    public static class KeyValuePairEnumerableExtensions
+    internal static class KeyValuePairEnumerableExtensions


This isn't a breaking change because it was introduced since the last release, yes?

Yes, that's just one from the previous PR.

jskeet · 2021-06-24T09:49:37Z

apis/Google.Cloud.Diagnostics.Common/Google.Cloud.Diagnostics.Common/Logging/GoogleLogger.cs

+        {
+        }
+
+        [Obsolete("Added for backward compatibility only when moving GoogleLogger to Common.")]


I'm somewhat lost as to whether we actually still need this. Given that it's internal, I'd expect to be able to remove it?

This is the one remaining piece that allows us to use the Common.GoogleLogger from the AspNetCore.GoogleLogger taking into account some of the obsolte types that remained in AspNetCore* that were not moved to Common. This is called from Common.GoogleLoggleProvider which is called from AspNetCore.GoogleLoggerProvider. I added the Obsolete attribute so that we ourselves know not to depend on it.

amanda-tarafa

Addressed all comments and fixed the language version mismatch. Will wait on green for merging.

amanda-tarafa · 2021-06-24T14:30:53Z

...stics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.IntegrationTests/Logging/LoggingTest.cs

@@ -545,11 +547,11 @@ public async Task Logging_Trace_External_OneEntry(TestServer server)
                LogEntry entry = Assert.Single(results);

                // And the resource name of the trace associated to it contains the external trace id.
-                Assert.Contains(TestEnvironment.GetTestProjectId(), entry.Trace);
-                Assert.Contains("external_trace_id", entry.Trace);
+                      Assert.Contains(TestEnvironment.GetTestProjectId(), entry.Trace);


amanda-tarafa · 2021-06-24T14:31:54Z

...Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Tests/Logging/GoogleLoggerTest.cs

@@ -67,10 +67,12 @@ private string Formatter(string message, Exception ex)
            Common.LoggerOptions options = Common.LoggerOptions.CreateWithServiceContext(
                logLevel, logName, labels, monitoredResource, retryOptions: retryOptions, serviceName: serviceName, version: version);
 #pragma warning disable CS0618 // Type or member is obsolete


Yes, this is the one that is used to keep backward compatibility.

amanda-tarafa · 2021-06-24T14:37:23Z

...Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Tests/Logging/GoogleLoggerTest.cs

            string fullTraceName = TraceTarget.ForProject(ProjectId).GetFullTraceName(traceId);

            Predicate<IEnumerable<LogEntry>> matcher = logEntries =>
            {
                LogEntry entry = logEntries.Single();
                return entry.LogName == new LogName(ProjectId, BaseLogName).ToString() &&
                    entry.Trace == fullTraceName &&
-                    entry.SpanId == spanId;
+                    entry.SpanId == $"{spanId:x16}";


Yes, the now obsolete TraceContextForLogEntry has a string for SpanId, and docs saying that it should be in base 16. But now we use the new ITraceContext where span ID is a ulong, and for setting it in the entry we are the ones who have to convert it to base 16.

amanda-tarafa · 2021-06-24T14:39:12Z

...iagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore/Logging/TraceContextForLogEntry.cs

@@ -33,6 +33,7 @@ namespace Google.Cloud.Diagnostics.AspNetCore
    /// These values can be attached to a log entry to establish the
    /// relation of it and a trace.
    /// </summary>
+    [Obsolete("Use Common.ITraceContext instead.")]


amanda-tarafa · 2021-06-24T14:39:34Z

...Cloud.Diagnostics.Common/Google.Cloud.Diagnostics.Common/KeyValuePairEnumerableExtensions.cs

@@ -23,7 +23,7 @@ namespace Google.Cloud.Diagnostics.Common
    /// Extension methods for converting KeyValuePair Enumerables to several types,
    /// including Protobuf well known types.
    /// </summary>
-    public static class KeyValuePairEnumerableExtensions
+    internal static class KeyValuePairEnumerableExtensions


Yes, that's just one from the previous PR.

amanda-tarafa · 2021-06-24T14:42:54Z

apis/Google.Cloud.Diagnostics.Common/Google.Cloud.Diagnostics.Common/Logging/GoogleLogger.cs

+        {
+        }
+
+        [Obsolete("Added for backward compatibility only when moving GoogleLogger to Common.")]


This is the one remaining piece that allows us to use the Common.GoogleLogger from the AspNetCore.GoogleLogger taking into account some of the obsolte types that remained in AspNetCore* that were not moved to Common. This is called from Common.GoogleLoggleProvider which is called from AspNetCore.GoogleLoggerProvider. I added the Obsolete attribute so that we ourselves know not to depend on it.

jskeet · 2021-06-24T14:57:50Z

Looks like there are a couple of doc-comment issues and some compatibility issues. I'm happy to look at the compatibility ones if you'd like.

This allows easier use of GoogleLogger in non ASP.NET Core applications. Towards googleapis#6367 - Replicate LoggerOptions in Common. - And have AspNetCore*.LoggerOptions be just a wrapper of Common.LoggerOptions. - Copies ILogEntryLabelProvider to Common. - And marks the one in AspNetCore* Obsolete. - Makes AspNetCore*.IExternalTraceProvider obsolete. - It can now be replaced by Common.ITraceContext.

amanda-tarafa · 2021-06-24T15:06:51Z

I'm looking, but the doc comments one are not a problem on local, so I'm not sure what's happening.

amanda-tarafa · 2021-06-24T15:12:21Z

Doc-comment issues fixed.

The compaibility differences I'm pretty sure are not a problem, but a second pair of eyes would be great. I think is just the tool that's being more strict than it needs to.

amanda-tarafa · 2021-06-24T15:20:05Z

Interface 'ILogEntryLabelProvider' implemented interface added 'ILogEntryLabelProvider'
Interface 'ILogEntryLabelProvider'; method 'void Invoke(Dictionary<String, String> labels)' removed.

These are:

AspNetCore.ILogEntryLabelProvider now implementes Common.ILogEntryLabelProvider.
The method was removed from AspNetCore.ILogEntryLabelProvider because it's now in Common.ILogEntryLabelProvider.

jskeet · 2021-06-24T15:38:50Z

Merging with admin rights. Although the breaking changes are theoretically breaking, we don't expect them to actually cause any issues in this case. Will note this in the release notes.

amanda-tarafa requested a review from jskeet June 24, 2021 01:03

amanda-tarafa self-assigned this Jun 24, 2021

amanda-tarafa requested a review from a team as a code owner June 24, 2021 01:03

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2021

jskeet approved these changes Jun 24, 2021

View reviewed changes

amanda-tarafa force-pushed the diag-logging-to-common branch from b6cf5f3 to 8c05176 Compare June 24, 2021 14:52

amanda-tarafa commented Jun 24, 2021

View reviewed changes

amanda-tarafa force-pushed the diag-logging-to-common branch from 8c05176 to e30a1f1 Compare June 24, 2021 15:02

jskeet merged commit 60e8cd8 into googleapis:master Jun 24, 2021

amanda-tarafa deleted the diag-logging-to-common branch July 5, 2021 11:51

amanda-tarafa mentioned this pull request Jul 5, 2021

fix: Fully qualifies all alternative types/members for obsolete ones. #6711

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds GoogleLogger to Diagnostics.Common #6657

feat: Adds GoogleLogger to Diagnostics.Common #6657

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

jskeet left a comment

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet Jun 24, 2021

amanda-tarafa Jun 24, 2021

amanda-tarafa left a comment

amanda-tarafa Jun 24, 2021

amanda-tarafa Jun 24, 2021

amanda-tarafa Jun 24, 2021

amanda-tarafa Jun 24, 2021

amanda-tarafa Jun 24, 2021

amanda-tarafa Jun 24, 2021

jskeet commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

jskeet commented Jun 24, 2021

feat: Adds GoogleLogger to Diagnostics.Common #6657

feat: Adds GoogleLogger to Diagnostics.Common #6657

Conversation

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

amanda-tarafa commented Jun 24, 2021

jskeet commented Jun 24, 2021