GoogleStackdriverTarget for NLog - Added SendJsonPayload property #2256

snakefoot · 2018-06-11T21:53:50Z

Allows you to do this:

    <target name="stackdriver" xsi:type="GoogleStackdriver" projectId="PROJECT_ID" logId="LOG_ID" includeEventProperties="true" sendJsonPayload="true">
      <contextproperty name="exception" layout="${exception:format=tostring}" />
    </target>

snakefoot · 2018-06-11T21:55:45Z

@chrisdunelm Thought that NLog should also support your LogEntry.JsonPayload

chrisdunelm

Thanks for the PR :)
Mostly looks good, just a few minor comments; and one question.

apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog.Tests/GoogleStackdriverTargetTest.cs

+                }, includeEventProperties: true);
+            Assert.Single(uploadedEntries);
+            var entry0 = uploadedEntries[0];
+            Assert.Equal(string.Empty, entry0.TextPayload?.Trim() ?? string.Empty);


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

-                    InternalLogger.Warn(ex, "GoogleStackdriver(Name={0}): Exception at BuildLogEntry with Key={1}", Name, combinedProperty.Key);
-                    logEntry.Labels[combinedProperty.Key] = "null";
+                    if (string.IsNullOrEmpty(combinedProperty.Key))
+                        continue;


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

                {
-                    logEntry.Labels[combinedProperty.Key] = combinedProperty.Value?.ToString() ?? "null";
+                    if (string.IsNullOrEmpty(combinedProperty.Key))
+                        continue;


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

                }
-                catch (Exception ex)
+
+                logEntry.JsonPayload = jsonStruct;


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/readme.md

@@ -33,6 +33,7 @@ When using default options, logs will appear under these filter settings in the
 * **IncludeCallSiteStackTrace** - Include LogEntrySourceLocation extracted from NLog callsite (Also include filename and linenumber)
 * **IncludeEventProperties** - Includes structured logging properties from NLog LogEventInfo.Properties
 * **IncludeMdlc** - Include async thread context properties from NLog MappedDiagnosticsLogicalContext
+* **SendJsonPayload** - Instead of sending properties as custom labels, then they are sent as Json-Properties


manxjason · 2018-06-22T16:24:49Z

Any ETA on this? Exactly what I was waiting for.

…llections)

snakefoot · 2018-06-22T18:09:10Z

@manxjason Do you have any preference on having the Labels filled when using JsonPayload ?

manxjason · 2018-06-22T18:11:16Z

@snakefoot No preference at this point - perhaps in the future but use of Stackdriver is new to us

chrisdunelm

I like the extra collection support :)
Just a few changes requested...

apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog.Tests/GoogleStackdriverTargetTest.cs

+        }
+
+        [Fact]
+        public async Task SingleLogEntryWithJsonCollectionProperties()


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

+                        }
+                        return Value.ForStruct(dictionaryStruct);
+                    }
+                    else if (objectValue is System.Collections.ICollection collection)


apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

+                    if (objectValue is System.Collections.IDictionary dictionary)
+                    {
+                        var dictionaryStruct = new Struct();
+                        var itemEnumerator = dictionary.GetEnumerator();


…numerable)

apis/Google.Cloud.Logging.NLog/Google.Cloud.Logging.NLog/GoogleStackdriverTarget.cs

+                            }
+
+                            var itemTypeCode = Convert.GetTypeCode(itemEnumerator.Value);
+                            dictionaryStruct.Fields.Add(dictionaryKey, BuildSimpleJsonValue(itemTypeCode, itemEnumerator.Value));


chrisdunelm · 2018-06-22T20:31:01Z

@snakefoot From your point of view is this now ready for merging and release?

snakefoot · 2018-06-22T20:31:32Z

@chrisdunelm Yes it looks good to me.

chrisdunelm · 2018-06-22T20:34:48Z

Great. I'll wait for all CI to go green, then merge. I expect we'll do another beta release of this next week. Thanks very much :)

snakefoot · 2018-06-27T20:55:58Z

Nuget package is now available:

https://www.nuget.org/packages/Google.Cloud.Logging.NLog/2.0.0-beta02

manxjason · 2018-06-28T16:21:55Z

@snakefoot this is more an Issue but as this PR is fresh and you made it, perhaps you don't mind discussing the following?

Do you have any preference on having the Labels filled when using JsonPayload ?

Hmm i'm starting to think I should have said 'Yes'. Right now we're finding that the labels are being filled, but not with the actual values, but with the type instead for some reason.

So as per the documentation

Order order = new Order
{
    OrderId = 25544,
    Status = OrderStatus.Processing
};

logger.Info("Test {@value1}", order); // object Result:  Test {"OrderId":2, "Status":"Processing"}

Results in looking like this: https://imgur.com/Olav4qc. Note the message format and the Property's value is the name of the type.

Our setup looks like:

GoogleStackdriverTarget googleTarget = new GoogleStackdriverTarget
{
    ProjectId = "234234",
    Name = "desktop app",
    CredentialFile = Path.Combine(@"C:\asdasd.json"),
    Layout = layout,
    IncludeEventProperties = true,
    ContextProperties = ....
{

And our layout:

JsonLayout layout = new JsonLayout
            {
                Attributes =
                {
                    new JsonAttribute("message", "${message}"),
                    new JsonAttribute("type", "${exception:format=Type}"),
                    new JsonAttribute("exception", "${exception:format=Message}"),
                    new JsonAttribute(
                        "innerException",
                        new JsonLayout
                        {
                            Attributes =
                            {
                                new JsonAttribute(
                                    "type",
                                    "${exception:format=:innerFormat=Type:MaxInnerExceptionLevel=1:InnerExceptionSeparator=}"),
                                new JsonAttribute(
                                    "message",
                                    "${exception:format=:innerFormat=Message:MaxInnerExceptionLevel=1:InnerExceptionSeparator=}")
                            },
                            RenderEmptyObject = false,
                            IncludeAllProperties = true,MaxRecursionLimit = 100
                        })
                },
                RenderEmptyObject = false,
                IncludeAllProperties = true, MaxRecursionLimit = 100
            };

snakefoot · 2018-06-28T18:49:54Z

@manxjason Not sure I understand why you are using the JsonLayout (It doesn't help when using SendJsonPayload). I would instead put the values you have added with each of the JsonAttributes into the GoogleStackdriverTarget.ContextProperties

I first implemented the NLog-GoogleStackdriverTarget to have the same features as for log4net. And then I added the support for JsonPayload so it would have the same features as for Serilog.

I don't have much experience with the Google Logging Framework to know what works best for you.

Are you missing an RenderEmptyObject option on GoogleStackdriverTarget.ContextProperties ?

snakefoot · 2018-06-29T18:09:10Z

@manxjason Discovered your use of POCO-objects, so created #2300 to support this.

GoogleStackdriverTarget for NLog - Added SendJsonPayload property

e5d5e33

snakefoot force-pushed the master branch from 4141eb6 to e5d5e33 Compare June 11, 2018 22:00

jskeet assigned chrisdunelm Jun 22, 2018

chrisdunelm reviewed Jun 22, 2018

View reviewed changes

snakefoot added 2 commits June 22, 2018 20:05

GoogleStackdriverTarget for NLog - Added SendJsonPayload property (Co…

193f187

…llections)

GoogleStackdriverTarget for NLog - Added SendJsonPayload property (docs)

d13c419

chrisdunelm reviewed Jun 22, 2018

View reviewed changes

GoogleStackdriverTarget for NLog - Added SendJsonPayload property (IE…

f5636fb

…numerable)

snakefoot force-pushed the master branch from c9e98fc to f5636fb Compare June 22, 2018 20:01

chrisdunelm reviewed Jun 22, 2018

View reviewed changes

chrisdunelm merged commit c8e228c into googleapis:master Jun 22, 2018

snakefoot mentioned this pull request Jun 29, 2018

GoogleStackdriverTarget for NLog - Added SendJsonPayload Object Properties #2300

Closed

GoogleStackdriverTarget for NLog - Added SendJsonPayload property #2256

GoogleStackdriverTarget for NLog - Added SendJsonPayload property #2256

Conversation

snakefoot commented Jun 11, 2018 • edited

snakefoot commented Jun 11, 2018

chrisdunelm left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

manxjason commented Jun 22, 2018

snakefoot commented Jun 22, 2018

manxjason commented Jun 22, 2018

chrisdunelm left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

chrisdunelm commented Jun 22, 2018

snakefoot commented Jun 22, 2018

chrisdunelm commented Jun 22, 2018

snakefoot commented Jun 27, 2018

manxjason commented Jun 28, 2018

snakefoot commented Jun 28, 2018 • edited

snakefoot commented Jun 29, 2018

snakefoot commented Jun 11, 2018 •

edited

snakefoot commented Jun 28, 2018 •

edited