GoogleCloudAppender - Allow jsonPayload to be sent instead of textPayload

[knodel12]

Is your feature request related to a problem? Please describe.
Stackdriver normally allows for nicely formatted data via a jsonPayload. This library does not have any support for that currently it seems.

Describe the solution you'd like
Ideally, GoogleCloudAppender could be extended and the text payload could be transformed to a json payload in the implementer code base. Why would all of the classes for log4net be sealed, preventing this as a possibility?

Could also just be a configuration option to the appender instead specifying to use jsonPayload instead of textPayload.

Describe alternatives you've considered
The only thing seems to be branching this repository and changing the underlying logic to no longer utilize textPayload directly.

Additional context

[jskeet]

I'll try to have a look next week (tomorrow and Monday are UK public holidays). The choice to make most classes sealed is a very deliberate one: the .NET client libraries team all follow the guidance of "design for inheritance or prohibit it" - where most situations don't call for inheritance (and allowing inheritance constrains the implementation and evolving API surface significantly).

[knodel12]

@jskeet Thanks. As for extending, StackDriver is quite a bit more flexible than log4net is. Allowing an extension of the class to change the payload to StackDriver would allow for quite a bit more flexibility in the consuming application compared to configuration in this case. It seems difficult to encompass a constantly changing API in a project like this.

All of the logic to do this work through log4net patterns has been very solid. We've had a good experience overall with the library, but adding jsonPayload support would help the team quite a bit.

[jskeet]

There are ways of adding flexibility without using inheritance. We'll look next week, but I'd be very surprised if we decided inheritance was the best option here.

[knodel12]

If I may ask, what are the some of the potential solutions to this? I'll be following this ticket to see what the results of your investigation are. Appreciate the prompt feedback.

[jskeet]

I can't say without looking at the code, and it's nearly 10pm on Good Friday for me, so I'm definitely not going to look now. Please be patient and wait until next week.

[jskeet]

Okay, I've now had a quick look, and I think it should be quite straightforward given the way log4net configuration works:

First, we introduce an IJsonLayout along these lines:

public interface IJsonLayout
{
    Google.Protobuf.WellKnownTypes.Struct Format(LoggingEvent loggingEvent);
}

Then we add a JsonLayout property of type IJsonLayout to GoogleStackdriverAppender. When writing a log entry (other than the ones for "we've lost logs"), if the JsonLayout property is non-null, we ask that to format the event first as a Struct. We use the existing rendering via TextPayload if either there's no JsonLayout configured, or it returns null for the event.

That allows you to write arbitrary code, with additional configuration in the normal way. It separates the concerns of "formatting the event" and "appending the event" quite cleanly IMO, and feels idiomatic to log4net - although I'll readily admit I don't have much experience of log4net.

I've prototyped this and it works as expected - any objections before we move to the full implementation?

[knodel12]

@jskeet I like the approach. Would I be able to specify the setting in the configuration or just in code?

If I upgrade the library is there a chance that some fields may change in the LoggingEvent passed in, or what will be populated in that object? I set up logging for the appender for errors, but that wasn't initially very clear on how to do. It was mainly a complex issue because of log4net's design.

[jskeet]

Would I be able to specify the setting in the configuration or just in code?

Yes, you can specify it in configuration, and that can contain additional configuration - just like how text layouts are specified.

If I upgrade the library is there a chance that some fields may change in the LoggingEvent passed in, or what will be populated in that object?

I'm afraid I don't understand the question. The GoogleStackdriverAppender is passed a LoggingEvent by log4net, and we don't modify it in any way as far as I can see. (Your IJsonLayout implementation shouldn't do so either.)

[knodel12]

That makes much more sense. My mistake, I thought that was the stack driver object. This seems like a fantastic solution. Thanks for explaining!

[jskeet]

Reopening until the code is released - which I hope will be tomorrow, at least as a beta.

[knodel12]

This looks like a very easy to implement solution. I'm glad there's a fallback if IJsonLayout ends up returning null. It seems that that interface implementation needs to be very careful to not throw an exception, correct?

[jskeet]

It seems that that interface implementation needs to be very careful to not throw an exception, correct?

Well, in the same way that any layout shouldn't throw an exception. You're right that we don't explicitly try to handle it within the appender. If that proves a problem, we could potentially add that later without an API surface change.

The NuGet package (3.1.0) is now available, by the way.