Remove override of Write(AsyncLogEventInfo)

[snakefoot]

There is no need for SignalRTarget to override this method:

protected override void Write(AsyncLogEventInfo logEvent)

Especially since it breaks the contract of calling AsyncLogEventInfo.Continuation on success and failure.

Removing the override will fix it.

[toddmeinershagen]

@snakefoot Thanks for the feedback. Would you be able to provide a unit test that would cause that exception on continuation. I will make the change and add the test to my suite of tests to ensure the fix addresses the issue you are bringing up. Thanks...

[snakefoot]

If you put the SignalRTarget inside the NLog-RetryingWrapper. Then ensure that the SignalRTarget always fails in the unit-test.

Then you would see that the NLog-RetryingWrapper doesn't attempt a second retry.

[304NotModified]

good work @snakefoot

It this also the case as this library has a dependency on NLog 3?

Would recommend to update also NLog's dependency to 4.4.2

[toddmeinershagen]

thanks @304NotModified for the idea of upgrading to 4.4.2. I was trying to write a test with the retrying wrapper and a failing signalr target. I wasn't able to demonstrate an issue with a continuation. Could you provide a code sample that creates this scenario?

[snakefoot]

@toddmeinershagen Sorry for the delay, but just returned from a 2 week vacation. I can see my suggestion for an NLog-test was bad, as the NLog actually implements an exception-fallback in case the custom-target fails to uphold the contract for AsyncLogEventInfo (But not for the sucess-case).

An easier to test would be to make the following Log-call:

var ev = new NLog.LogEventInfo(NLog.LogLevel.Debug, "MyLogger", "hello world"));
ev.Properties["MyProperty"] = "goodbye world";
logger.Log(ev);

Then use the following target layout "${event-properties:item=MyProperty}", and see that it fails to print the property-value. Removing the override of Write(AsyncLogEventInfo) will make it work (As NLog-core will correctly call Target.MergeEventProperties).

[toddmeinershagen]

@snakefoot Thanks for getting back to me. I tried adding the following test to my suite, but it too passes.

Perhaps I am not setting up the configuration properly?

[Test]
public void given_nlog_configured_to_use_signalr_target_for_hub_when_logging_event_should_log_to_signalr2()
{
    var target = new SignalRTarget
    {
        Name = "signalr",
        Uri = HubBaseUrl,
        Layout = "${message}${event-properties:item=MyProperty}"
    };

    var asyncWrapper = new AsyncTargetWrapper(target);
    SimpleConfigurator.ConfigureForTargetLogging(asyncWrapper);

    const string expectedMessage = "Hello, world.";
    const string expectedProperty = "Goodbye, world.";

    var logEvent = new LogEventInfo(LogLevel.Info, "MyLogger", expectedMessage);
    logEvent.Properties["MyProperty"] = expectedProperty;
    Logger.Log(logEvent);

    Wait.Until(() => Test.Current.SignalRLogEvents.FirstOrDefault(x => x.Level == "Info") != null, TimeSpan.FromSeconds(10));

    Test.Current.SignalRLogEvents.Should().Contain(x => x.Level == "Info" && x.Message == expectedMessage + expectedProperty);
}

[snakefoot]

@toddmeinershagen Change Logger.Log into Logger.Info

[toddmeinershagen]

@snakefoot I changed the method from Log() to Info() and it did cause the test to fail. Unfortunately, removing the async overload method for Write() did not fix it.

Any ideas?

[toddmeinershagen]

@snakefoot I upgraded the project to point to the v3.2.1 of NLog (i don't want to force users to have to upgrade their projects to v4.x.x for no reason unless they want to) and changed the Layout property to not include the Message as you had originally suggested.

With or without the async method override, the output works properly. Any thoughts on a test that can cause the Continuation to fail so that I can demonstrate that this override is detrimental?

[Test]
public void given_nlog_configured_to_use_signalr_target_for_hub_when_logging_event_should_log_to_signalr2()
{
    var target = new SignalRTarget
    {
        Name = "signalr",
        Uri = HubBaseUrl,
        Layout = "${event-properties:item=MyProperty}"
    };

    var asyncWrapper = new AsyncTargetWrapper(target);
    SimpleConfigurator.ConfigureForTargetLogging(asyncWrapper);

    const string expectedMessage = "Hello, world.";
    const string expectedProperty = "Goodbye, world.";

    var logEvent = new LogEventInfo(LogLevel.Info, Logger.Name, expectedMessage);
    logEvent.Properties["MyProperty"] = expectedProperty;
    Logger.Info(logEvent);

    Wait.Until(() => Test.Current.SignalRLogEvents.FirstOrDefault(x => x.Level == "Info") != null, TimeSpan.FromSeconds(10));

    Test.Current.SignalRLogEvents.Should().Contain(x => x.Level == "Info" && x.Message == expectedProperty);
}

[toddmeinershagen]

Actually - ignore my last post. I stopped using the AsyncTargetWrapper, and it demonstrated the problem that you were expressing.

I will check this code in and release a new version of the assembly today. Thanks!

[304NotModified]

About the NLog dependency, NLog 4 is 1,5 year stable and pointing to an old version just look unmaintained IMO. Would recommended at least 4.0.1.

[toddmeinershagen]

Hi Julian,

My version doesn't prohibit people from using the v4, it just doesn't force them to. Is there a good reason why I need to force users to upgrade to v4?

Todd

On Sat, Feb 25, 2017 at 2:10 PM, Julian Verdurmen notifications@github.com wrote:

About the NLog dependency, NLog 4 is 1,5 year stable and pointing to an old version just look unmaintained IMO. Would recommended at least 4.0.1.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/toddmeinershagen/NLog.SignalR/issues/4#issuecomment-282509044, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK1ZJua4-0pA1-hJ7BEvdGPfVZLU2YFks5rgIqvgaJpZM4L3T0J .

[304NotModified]

It's unmaintained and unsupported.

But its a matter of style. I could understand why you would stick to an older version.