Feature/LOG4NET-644 #56
Conversation
…est Service. It's required for Newtonsoft.Json Package from Nuget.org.
There was a problem hiding this comment.
First, I want to thank you for the time and effort which went into this.
I feel like there is probably scope for a generic REST appender for log4net -- we have other common appenders for other storage mechanisms like files and databases.
However, I think there are some items to consider here, particularly around reliability of logging. Someone using this is going to expect that an intermittent network error doesn't discard their logs. Unlike with ADO, where the target database is often on the same machine or at least local network (and if the db goes down, the app is in much bigger trouble), REST targets are more likely to present connectivity issues.
Long ago, I wrote a splunk appender (https://github.com/chillisoft/splunk4net) which uses a local sqlite file for an intermediatary store. It might provide some ideas on buffering. It's not a good candidate for log4net inclusion precisely because it depends on extra libraries that I wouldn't want a log4net consumer to have to have installed (namely the SQLite and Splunk packages, which are only of interest to consumers of this specific appender). It also assumes that it can write to local files, which may not be the case for a hosted website.
In addition, I'd expect that someone might want to use this to log to a service like Sumo or Splunk, so there really needs to be support for authorisation. And, unlike the ADO appender, this implementation doesn't extend the Buffering appender -- which is something else that should (probably) change: as a consumer, I'd expect to be able to set up buffering for any appender performing I/O.
Right now, this appender seems, to me, to be a good candidate for a standalone plugin nuget package, especially since there aren't a lot of resources to spare for log4net at the moment. Once that nuget package stands up to varied use, it might be a candidate for inclusion. I'm wary of adding new features right now -- I'm trying to first stablise, address any issues I can see, and clean up the issue & PR queues.
| /// <see cref="FormatValue"/>. | ||
| /// </para> | ||
| /// </remarks> | ||
| public class RestAppenderParameter |
There was a problem hiding this comment.
please use one file per class -- just makes it easier to find stuff when searching through a project.
|
|
||
| requestMessage.Content = new StringContent(messageContent); | ||
|
|
||
| var result = _httpClient.SendAsync(requestMessage).Result; |
There was a problem hiding this comment.
minor: result is never used
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Sadly, your connection is bad or rest endpoint does not exist |
There was a problem hiding this comment.
Unfortunately, this means that an intermittent connection error loses the log. I expect that people would probably like to use REST logging to services which are on another machine, perhaps even on a totally different network, so, unlike ADO appending, I expect the chance of failure to be higher. It would be nice if there were some kind of contingency plan here.
I've create a Rest Appender based on Ado.Net Appender.
Unit tests and XML Samples are included the code repository.
Newtonsoft JSON library added as dependency for rest content formating.