Improve EventLogTarget installation logic - handle MaximumKilobytes for apps without elevated privileges

[TheCorio]

Remove MaximumKilobytes setting from GetEventLog(), because when app is run without elevated privileges, the method spams exceptions.
Move MaximumKilobytes setting to installation (and initialization) phase, when the application is typically run with elevated privileges.
Add logic to not override a bigger value of MaximumKilobytes, because the Event log can be used by multiple Sources, each having its own space requirements.

[codecov]

Codecov Report

Merging #3017 into dev will decrease coverage by <1%.
The diff coverage is 71%.

@@          Coverage Diff           @@
##             dev   #3017    +/-   ##
======================================
- Coverage     80%     80%   -<1%     
======================================
  Files        338     341     +3     
  Lines      26615   27014   +399     
  Branches    3521    3586    +65     
======================================
+ Hits       21261   21574   +313     
- Misses      4356    4419    +63     
- Partials     998    1021    +23

[TheCorio]

Hi again, @304NotModified, what are the expected actions to address the "codecov/patch" check?

[304NotModified]

it warns us for the coverage on the new code, it's 17% instead of 50%.

the code is never touched with an unit test, would be nice if we could do that:

[TheCorio]

Hi, @304NotModified. Could you please check if the codecov/patch actually ran on 84319d6?

[snakefoot]

Think it would be nicer if you implemented an internal constructor that took an internal IEventLogWrapper-interface as input parameter. The default constructor would then call the new internal constructor with a private-class-implementation of the IEventLogWrapper-interface. Ofcourse the unit-tests will not be able to test initialization from NLog.config, but I think that is okay. protected internal means that it becomes part of the public interface.

[TheCorio]

I get your point on injecting the implementation in the constructor instead of the property. But you mentioned an IEventLogWrapper, do you suggest merging the IEventLogStaticWrapper and IEventLogInstanceWrapper?

Also, you think these new interfaces and implementations should not fall under protected internal?

[snakefoot]

do you suggest merging the IEventLogStaticWrapper and IEventLogInstanceWrapper?

Yes I would prefer just a single interface. And this interface and the constructor getting it as parameter are both marked as internal-only (Not protected).

[TheCorio]

Hi, @snakefoot. I've addressed your comments in 5017749, but the build failed. Can you, or maybe @304NotModified, assist?

[snakefoot]

Was thinking that all public constructors should call the unified internal constructor:

        private readonly IEventLogWrapper _eventLogWrapper;

        /// <summary>
        /// Initializes a new instance of the <see cref="EventLogTarget"/> class.
        /// </summary>
        public EventLogTarget()
            : this(null, null)
        {
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="EventLogTarget"/> class.
        /// </summary>
        /// <param name="name">Name of the target.</param>
        public EventLogTarget(string name)
            : this(null, null)
        {
            Name = name;
        }

        /// <summary>
        /// Initializes a new instance of the <see cref="EventLogTarget"/> class.
        /// </summary>
        public EventLogTarget(IAppDomain appDomain)
            : this(null, appDomain)
        {
        }

        internal EventLogTarget(IEventLogWrapper eventLogWrapper, IAppDomain appDomain)
        {
            _eventLogWrapper = eventLogWrapper ?? new EventLogWrapper()
            appDomain = appDomain ?? LogFactory.CurrentAppDomain;
            Source = appDomain.FriendlyName;
            Log = "Application";
            MachineName = ".";
            MaxMessageLength = EventLogMaxMessageLength;
            OptimizeBufferReuse = GetType() == typeof(EventLogTarget);  // Class not sealed, reduce breaking changes
        }

[snakefoot]

@Coriolanuss I know it is weird with the constructor public EventLogTarget(IAppDomain appDomain) but sadly enough it has become part of the public interface. Removing it would be a breaking change, and has to wait for NLog 5.0

We have to keep it until NLog ver. 5.0 where it can be removed. You are welcome to add the obsolete attribute. Ex:

        [Obsolete("This constructor will be removed in NLog 5. Marked obsolete on NLog 4.6")]
        public EventLogTarget(IAppDomain appDomain)
            : this(null, appDomain)
        {
        }

[TheCorio]

Hi @snakefoot, I have brought back the constructor. Any other suggestions?

[snakefoot]

No I think it looks great. Especially with the improved tests. But I don't have the final verdict :)

[TheCorio]

It's nice to hear.
@304NotModified, do you have any comments?

[304NotModified]

Looks good! please move the new interfaces/classes (the interface to the "internal" folder and namespace) to new files (don't forget to copy the license header) and then I will merge this one.

PS: builds failed, not sure what's wrong but restarted all of them

[304NotModified]

No I think it looks great. Especially with the improved tests. But I don't have the final verdict :)

You approval says a lot anyway 👍

[304NotModified]

please move the new interfaces/classes (the interface to the "internal" folder and namespace) to new files (don't forget to copy the license header)

[304NotModified]

please move the new interfaces/classes (the interface to the "internal" folder and namespace) to new files (don't forget to copy the license header)

Maybe that isn't that easy with the multi projects now, so fine for now :)

[304NotModified]

Merged, thanks for the contribution and unit tests!