Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

gscx233
Copy link

@gscx233 gscx233 commented Mar 31, 2017

ReadFrom.AppSettings() can only accept string input. So add a overload method to accept string params.

Copy link
Collaborator

@wparad wparad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add in a unit test, as well as better document the use case.

I want to make sure we handle the general use case, this seems like a specific use of a configuration file, and there is another ticket which asks for something similar. We should make sure any change we do here will encourage a solution to the other one. (Although I'm not looking for solving #28 immediately)

Can you add to the PR conversation your configuration file as well as the code you are looking to execute, so that we can discuss what the best way to accomplish this is?

if (string.IsNullOrEmpty(region)) throw new ArgumentException(nameof(region));

CloudWatchSinkOptions options = new CloudWatchSinkOptions() { LogGroupName = logGroupName };
IAmazonCloudWatchLogs client = new AmazonCloudWatchLogsClient(accessKey, secretKey, Amazon.RegionEndpoint.GetBySystemName(region));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This forces an implementation of a IAmazonCloudWatchLogs client, which shouldn't be newed up here.

/// <param name="secretKey">AWS Secret Access Key.</param>
/// <param name="region">The region to connect.</param>
/// <returns></returns>
public static LoggerConfiguration AmazonCloudWatch(this LoggerSinkConfiguration loggerConfiguration, string logGroupName, string accessKey, string secretKey, string region)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a class to contain all the options we care about passing in, the way this currently is does not make it scalable.

@gscx233
Copy link
Author

gscx233 commented Mar 31, 2017

The config file is mostly the same as #28 :

<appSettings>    
    <add key="serilog:minimum-level" value="Verbose" />
    <add key="serilog:using:AmazonCloudWatch" value="Serilog.Sinks.AwsCloudWatch" />
    <add key="serilog:write-to:AmazonCloudWatch.logGroupName" value="***" />
    <add key="serilog:write-to:AmazonCloudWatch.accessKey" value="***" />
    <add key="serilog:write-to:AmazonCloudWatch.secretKey" value="***" />
    <add key="serilog:write-to:AmazonCloudWatch.region" value="us-east-1" />
</appSettings>

And the code is:

Log.Logger = new LoggerConfiguration()
                .ReadFrom.AppSettings()
                .CreateLogger();

@wparad
Copy link
Collaborator

wparad commented Mar 31, 2017

I may be missing something but could you help me figure out how using this method would allow setting the LogEventRenderer as part of the CloudWatchOptions configuration. This is an important feature we are offering, and it isn't clear from the Serilog documentation on how to do this. (I ask because the constructor we currently have wouldn't support this as is, and I don't want to diverge from the functionality that is currently being provided by the other constructor).

@wparad
Copy link
Collaborator

wparad commented Apr 24, 2017

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

@gscx233 gscx233 closed this Apr 25, 2017
@gscx233
Copy link
Author

gscx233 commented Apr 25, 2017

Closed because another PR #32 covered this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants