Create Overloads To Allow Configuration via AppSettings #32

chrisoverzero · 2017-04-19T16:54:32Z

The library "serilog-settings-configuration" allows Serilog to read
from configuration provided by
"Microsoft.Extensions.Configuration". This integration has particular
limitations, and the existing method AmazonCloudWatch cannot be
used.

Three overloads have been created in order to support this:

One allows the AWS access key, secret access key, and region to be
read from the environment. Because of this, an implementation of
IAmazonCloudWatchLogs with a default constructor (or a constructor
with all optional arguments) may be provided via settings. If none
is provided, an instance of AmazonCloudWatchLogsClient is
created.
Another takes in the region from settings, and reads the AWS access
key and secret access key from the environment. This unconditionally
creates an instance of AmazonCloudWatchLogsClient.
Another takes in all values from settings. This also unconditionally
creates an instance of AmazonCloudWatchLogsClient.

In all cases, the log group name is required, and the log event
renderer, the minimum log event level, the batch size limit, and the
period are optional, with default values. These default values are the
same as those used in CloudWatchSinkOptions.

The library "serilog-settings-configuration" allows Serilog to read from configuration provided by "Microsoft.Extensions.Configuration". This integration has particular limitations, and the existing method `AmazonCloudWatch` cannot be used. Three overloads have been created in order to support this: - One allows the AWS access key, secret access key, and region to be read from the environment. Because of this, an implementation of `IAmazonCloudWatchLogs` with a default constructor (or a constructor with all optional arguments) may be provided via settings. If none is provided, an instance of `AmazonCloudWatchLogsClient` is created. - Another takes in the region from settings, and reads the AWS access key and secret access key from the environment. This unconditionally creates an instance of `AmazonCloudWatchLogsClient`. - Another takes in all values from settings. This also unconditionally creates an instance of `AmazonCloudWatchLogsClient`. In all cases, the log group name is required, and the log event renderer, the minimum log event level, the batch size limit, and the period are optional, with default values. These default values are the same as those used in `CloudWatchSinkOptions`.

thoean

Thank you for the additions. They are good and I'm happy to merge them. However, I've added two general questions where I'd like to hear your thoughts before approving.

thoean · 2017-04-21T13:00:31Z

src/Serilog.Sinks.AwsCloudWatch/AwsCloudWatchConfigurationExtension.cs

 using Amazon.CloudWatchLogs;
 using Serilog.Configuration;
 using Serilog.Core;
 using Serilog.Events;
+using static Serilog.Sinks.AwsCloudWatch.CloudWatchSinkOptions;


While it makes some of the code concise, I also feel it makes it a lot harder to read. Any thoughts yourself of potentially removing it and making the explicit (while more verbose) reference to this class?

I do not mind matching house style on this.

thoean · 2017-04-21T13:06:28Z

src/Serilog.Sinks.AwsCloudWatch/AwsCloudWatchConfigurationExtension.cs

+        /// <param name="period">The period to be used when a batch upload should be triggered.</param>
+        /// <returns></returns>
+        /// <exception cref="ArgumentNullException"><paramref name="logGroupName"/> is <see langword="null"/>.</exception>
+        public static LoggerConfiguration AmazonCloudWatch(


I think we're creating 3 things here:

The CloudWatchSinkOption object.

The IAmazonCloudWatchLogs client.

The CloudWatchLogSink.

I think it'd be much easier and less error prone to provide 3 separate methods for these. While the current code can make it convenient for clients to use "yet another easy extension method", there's also a bunch of duplication like 3 times the same sink options being created.

What about we'd end up with the following additions to what we already have?

An extension method that creates the client, which allows to optionally specify the region.

An extension method that creates the options, with the overrides you added.

This will make it a 2-liner or potentially 3-liner for clients of this library, but I feel like it adds clarity and flexibility, and reduces potential future errors on this library.

Any thoughts on your side regarding these ideas?

I think perhaps you misunderstand.

The intent is not to have convenient overloads, the intent is to have machine-readable overloads. This is how discoverability in serilog-settings-configuration works. For example, in the Fulfiller Enablement squad (hi, fellow Cimpress employee), we effectively have this in the appsettings.json for a service:

"Serilog": { "Using": [ "Fen.Conventions" ], "MinimumLevel": "Debug", "WriteTo": { "Console": "Console", "SumoLogic": { "Name": "SumoLogic", "Args": { "endpointUrl": "<Sumo Logic endpoint omitted>", "sourceName": "pool_dev" } }, "CloudWatch": { "Name": "AmazonCloudWatch", "Args": { "logGroupName": "pool_dev", "accessKey": "<Merged in from secrets>", "secretAccessKey": "<Merged in from secrets>", "regionName": "eu-west-1" } } }, "Properties": { "Application": "Pool" }, "Enrich": [ "FromLogContext" ] }

_{(We currently have the code from this pull request in a utility library of ours, thus the "Using" of "Fen.Conventions".)}

In an object representing a sink configuration, the value for the "Name" key is the name of a LoggerSinkConfiguration extension method to call. The value for the "Args" key represents the arguments to an overload of the Named method. The values for those arguments are very limited: The serilog machinery can only read the following types of values:

numbers

strings

...not objects. It has special handling for some situations:

Converting a string to a URI

Converting a string to a TimeSpan

Converting a string (maybe also a number?) to a value of an enumeration

Converting a string that represents a fully-qualified type name to an instance of that type by calling a constructor with all optional arguments (which includes a no-argument constructor) only when bound to an argument whose type is an interface and that interface is implemented by the named type

I think that if you look at serilog-settings-configuration or some other libraries that already explicitly work with it, you'll see that this is a reasonable implementation, given the limitations.

What additional changes could you make to make point three work, i.e. can we turn the cloudwatch options into an interface?

Converting a string that represents a fully-qualified type name to an instance of that type by calling a constructor with all optional arguments (which includes a no-argument constructor) only when bound to an argument whose type is an interface and that interface is implemented by the named type

[...] can we turn the cloudwatch options into an interface?

I'm not sure the utility of that. Instead of specifying configuration values in configuration, that would be requiring the creation of implementations of ICloudWatchSinkOptions, each of which contains the configuration values for an environment. That would hinder the ability to merge appsettings.json (from base to environment-specific) files and to merge values in from user secrets or environment variables.

- Remove static imports

wparad · 2017-04-21T16:21:29Z

I'm on the fence here, personally using an appsettings file offends my idealistic soul as a software developer. There is something inherently unclean about storing the code for the logger configuration outside the method to set up the logger. I recognize however that some teams want to do this, but I'm interested in why the separation? What's wrong with just calling the extension methods in code as suggested, is there something that the appsettings file gets you that we aren't handling already?

I ask, because if there is something that can be improved in the library to make the code-first approach work, I would much prefer to see improvements there, rather than having new statements in the beautiful property settings only constructor.

chrisoverzero · 2017-04-21T17:28:14Z

@wparad

I agree that it's suboptimal, but it's how the library works. Even though the author of the Elasticsearch sink hides it deeply, that sink also news up a pre-chosen implementation of IElasticLowLevelClient. I've imagined an implementation of ReadFrom.Configuration that accepts both an IConfiguration and an IServiceProvider, so that arguments can be retrieved from dependency injection (say, IOptions<CloudWatchSinkOptions>), but I can't yet deploy my imagination to AWS.

[...] something that can be improved in the library to make the code-first approach work [...]

I think this is unlikely. The uninteresting configuration for a service we implemented in ASP.NET Core looks like this:

var builder = new ConfigurationBuilder()
    .SetBasePath(env.ContentRootPath)
    .AddJsonFile("appsettings.json", optional: true, reloadOnChange: true)
    .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true)
    .AddUserSecrets<StartupDevelopment>()
    .AddEnvironmentVariables();

That is, we read base configuration from an appsettings.json file, then merge it with environment-specific configuration, then merge it with user secrets, then merge it with environment variables. Perhaps some of the confusion comes from thinking of appsettings.json as a single source of configuration? It's a very flexible system; I even wrote an implementation that reads from Docker secrets (though I haven't gotten to use it anywhere yet).

wparad · 2017-04-24T14:44:46Z

Is this relevant? serilog/serilog-settings-configuration#53

chrisoverzero · 2017-04-24T14:57:44Z

When it's done, maybe? But according to the proposer, the hardest part is still hard -- you can't specify the implementation of IWhateverLoggingSystemClient directly through configuration except in limited circumstances.

That seems like a step backwards, anyway. Each implementer of a sink shouldn't have to go and parse JSON themselves. The library "serilog-settings-configuration" (or a variant/offshoot/descendant of it) should be moving closer to the standard configuration methods, not farther from them. Or so I believe.

gscx233 · 2017-04-25T02:54:51Z

@wparad

The reason why I want to store the logger configuration in a config file is to unify the code over different environments.

For example:
Assuming we have Development Testing and Production environment, and I want to use Serilog sinks in:

Development environment
- Log file sink
Testing environment
- Log file sink
- Event log sink
Production environment
- Event log sink
- AWS Cloud Watch Log sink

Then config file is the only option. I only need ReadFrom.AppSettings() in the code and use PowerShell to automatically transform config file for different environment during the automatic deploy process.

@chrisoverzero 's PR solves this problem, and hope my information helps.

thoean

I think we should continue on that path with a few additions / changes:

Please reset the version back to 2.0.0 - the build automatically sets the next version.
Reduce the complexity to a single additional method to avoid code duplication, and using default values instead. This will also make it easier to migrate to other systems later as people won't have adopted to too many variations of it.

Hope that's a reasonable and small change on your side.

chrisoverzero · 2017-04-25T16:02:55Z

@thoean

Please reset the version back to 2.0.0 - the build automatically sets the next version.

The build will change the version to 2.1.x? Or do you mean that you want the version to remain 2.0.x?

Reduce the complexity to a single additional method to avoid code duplication [...]

I have implemented this in the past, but the solution was unsatisfying. The logic to determine which constructor to call is less than straightforward, and it fails to communicate the contract of what combinations of { accessKey, secretAccessKey, regionName } are valid. (It's almost as though serilog-settings-configuration isn't a very satisfying library!) But I can do it and push it, and you can evaluate that.

- Decrease version number

thoean

Build version: No, the build will take 2.0.{buildnumber}. I tend to simplify by just using the 2.0.x version unless there are breaking changes.

Code duplication vs. if-statements: Interesting... I still feel like the one you have is cleaner.

I'm going to approve this PR. I have two small comments. Feel free to optionally include them.

thoean · 2017-04-26T09:35:00Z

src/Serilog.Sinks.AwsCloudWatch/AwsCloudWatchConfigurationExtension.cs

+            }
+
+            // If an invalid combination has been provided, yell.
+            throw new InvalidOperationException(


Should that be an ArgumentException?

It shouldn't be anything. There's no best choice for an exception here because it should not throw an exception. It should be the case that serilog-settings-configuration fails to find a valid overload (based on the provided args) and moves on without calling anything. But there's no way to exit the method by returning a LoggerConfiguration without creating a sink that I could find.

That said, I don't think so. It's not that a particular argument is wrong, it's that particular combinations are wrong, representing an invalid state.

thoean · 2017-04-26T09:39:13Z

src/Serilog.Sinks.AwsCloudWatch/AwsCloudWatchConfigurationExtension.cs

+                return new AmazonCloudWatchLogsClient(region);
+            }
+
+            if (accessKey == null && secretAccessKey == null)


You're missing the use case where only the access keys are provided, but not the region name.

I forgot that was a valid constructor. I'll get that in a jiff.

chrisoverzero · 2017-04-26T12:21:12Z

Code duplication vs. if-statements

That's "Overloads-and-duplication vs. all-optional-arguments-and-if-statements". Though if the constructor to AmazonCloudWatchClient were more convenient, this wouldn't be a thing. Or if Serilog could read from DI or something. I still like Serilog, despite these limitations!

Interesting... I still feel like the one you have is cleaner.

Cleanliness is not the only concern of software. The version as written now causes this library to adhere less well to the conventions of a Serilog sink. (And it's already a little slippery for having the default logging level be Informational.)

- Forgot a constructor

chrisoverzero · 2017-04-26T14:28:19Z

Though implementing that change made me realize that the supported constructors can simplify the overloads. How would you feel about only two overloads, with these signatures?

public static LoggerConfiguration AmazonCloudWatch(
    this LoggerSinkConfiguration loggerConfiguration,
    string logGroupName,
    string accessKey,
    string secretAccessKey,
    string regionName = null,
    ILogEventRenderer logEventRenderer = null,
    LogEventLevel minimumLogEventLevel = CloudWatchSinkOptions.DefaultMinimumLogEventLevel,
    int batchSizeLimit = CloudWatchSinkOptions.DefaultBatchSizeLimit,
    TimeSpan? period = null)

...and...

public static LoggerConfiguration AmazonCloudWatch(
    this LoggerSinkConfiguration loggerConfiguration,
    string logGroupName,
    string regionName = null,
    ILogEventRenderer logEventRenderer = null,
    LogEventLevel minimumLogEventLevel = CloudWatchSinkOptions.DefaultMinimumLogEventLevel,
    int batchSizeLimit = CloudWatchSinkOptions.DefaultBatchSizeLimit,
    TimeSpan? period = null)

This would allow following Serilog conventions more closely, communication that the keys must come as a pair, and reduction of some duplication.

thoean · 2017-04-26T19:12:08Z

Sounds good. Let's go with the 2 methods. A reasonable pragmatic approach to something where there's no right or wrong...

chrisoverzero · 2017-04-26T19:46:37Z

And done. Thanks for the feedback, even at – what time is it where you are? Like, 21:45?

👍

thoean requested changes Apr 21, 2017

View reviewed changes

Incorporate Pull Request Feedback

c04ec59

- Remove static imports

gscx233 mentioned this pull request Apr 25, 2017

Add overload method to enable CloudWatch Sink through config file. #30

Closed

thoean requested changes Apr 25, 2017

View reviewed changes

Incorporate Code Review Feedback

b79d165

- Decrease version number

thoean approved these changes Apr 26, 2017

View reviewed changes

Incorporate Code Review Feedback

8c3bbd9

- Forgot a constructor

Only Two Overloads, This Time

33428fe

thoean merged commit ee6fdb6 into Cimpress-MCP:master Apr 26, 2017

chrisoverzero deleted the feature/configuration branch April 26, 2017 20:15

gscx233 mentioned this pull request May 31, 2017

Sink does not support custom properties/enrichers. #37

Closed

thoean mentioned this pull request Jun 20, 2017

HOW TO: Pass Custom LogEventRenderer via Configuration? #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Overloads To Allow Configuration via AppSettings #32

Create Overloads To Allow Configuration via AppSettings #32

chrisoverzero commented Apr 19, 2017

thoean left a comment

thoean Apr 21, 2017

chrisoverzero Apr 21, 2017

thoean Apr 21, 2017

chrisoverzero Apr 21, 2017 •

edited

Loading

wparad Apr 21, 2017

chrisoverzero Apr 21, 2017

wparad commented Apr 21, 2017

chrisoverzero commented Apr 21, 2017

wparad commented Apr 24, 2017

chrisoverzero commented Apr 24, 2017

gscx233 commented Apr 25, 2017 •

edited

Loading

thoean left a comment

chrisoverzero commented Apr 25, 2017

thoean left a comment

thoean Apr 26, 2017

chrisoverzero Apr 26, 2017

thoean Apr 26, 2017

chrisoverzero Apr 26, 2017

chrisoverzero commented Apr 26, 2017 •

edited

Loading

chrisoverzero commented Apr 26, 2017 •

edited

Loading

thoean commented Apr 26, 2017

chrisoverzero commented Apr 26, 2017 •

edited

Loading

Create Overloads To Allow Configuration via AppSettings #32

Create Overloads To Allow Configuration via AppSettings #32

Conversation

chrisoverzero commented Apr 19, 2017

thoean 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

chrisoverzero Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wparad commented Apr 21, 2017

chrisoverzero commented Apr 21, 2017

wparad commented Apr 24, 2017

chrisoverzero commented Apr 24, 2017

gscx233 commented Apr 25, 2017 • edited Loading

thoean left a comment

Choose a reason for hiding this comment

chrisoverzero commented Apr 25, 2017

thoean 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

chrisoverzero commented Apr 26, 2017 • edited Loading

chrisoverzero commented Apr 26, 2017 • edited Loading

thoean commented Apr 26, 2017

chrisoverzero commented Apr 26, 2017 • edited Loading

chrisoverzero Apr 21, 2017 •

edited

Loading

gscx233 commented Apr 25, 2017 •

edited

Loading

chrisoverzero commented Apr 26, 2017 •

edited

Loading

chrisoverzero commented Apr 26, 2017 •

edited

Loading

chrisoverzero commented Apr 26, 2017 •

edited

Loading