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

ConvertToArrayFilter: System.ArgumentNullException: Value cannot be null #72

Open
DmitryMak opened this issue Nov 12, 2020 · 10 comments
Open

Comments

@DmitryMak
Copy link

DmitryMak commented Nov 12, 2020

We are getting this exception when trying to log with log4stash 3.0.0 and log4net 2.0.11.

log4net:ERROR [ElasticSearchAppender] ErrorCode: GenericFailure. Failed in DoAppend
System.ArgumentNullException: Value cannot be null.
Parameter name: input
   at System.Text.RegularExpressions.Regex.Split(String input)
   at log4stash.Filters.ConvertToArrayFilter.ValueToArray(String value)
   at log4stash.Filters.ConvertFilter.PrepareEvent(Dictionary`2 logEvent)
   at log4stash.ElasticAppenderFilters.PrepareEvent(Dictionary`2 logEvent)
   at log4stash.ElasticSearchAppender.PrepareAndAddToBulk(Dictionary`2 logEvent)
   at log4stash.ElasticSearchAppender.Append(LoggingEvent loggingEvent)
   at log4net.Appender.AppenderSkeleton.DoAppend(LoggingEvent loggingEvent)

Not sure if it is related but we use custom log4net code like
LogicalThreadContext.Stacks["FirstName"].Push("Alice")

With log4stash config like

<ElasticFilters>
    <Convert>
        <ToString>FirstName</ToString>
        <ToArray>
            <SourceKey>FirstName</SourceKey>
        </ToArray>

Edit: Code to reproduce the issue:

_log.Warn("Will be logged without FirstName");
using (LogicalThreadContext.Stacks["FirstName"].Push("Alice")) {
    _log.Warn("Will be logged with FirstName = Alice");
}
_log.Warn("This and further messages will not be logged due to ArgumentNullException in ConvertToArrayFilter");
@eran-gil
Copy link
Collaborator

eran-gil commented Nov 12, 2020

Try using this line instead:
LogicalThreadContext.Properties["FirstName"] = "Alice";

@DmitryMak
Copy link
Author

We rely on IDisposable returned from Push. Could you help us understand how this will fix the ArgumentNullException?

@eran-gil
Copy link
Collaborator

Look at https://github.com/urielha/log4stash/blob/master/src/log4stash/LogEvent/BasicLoggingEventParser.cs line 111 method.
It takes its values from the properties of the thread context and not the stack, these are different storages.
Therefore, you need to use properties with the current code.
Or, you can add a pull request with a way to sort it out.

@DmitryMak
Copy link
Author

DmitryMak commented Nov 12, 2020

Based on my experiments it is a bug ConvertToArrayFilter. When I turn it off by removing <ToArray> in the config, the exception disappears. The underlying issue that we were having also goes away. The ArgumentNullException prevented further logging. Removing <ToArray> filter fixed that.

So based on that, the issue is not related to Properties vs Stack. We've been using Stack for a long time and it always, and still, works.

@DmitryMak DmitryMak changed the title System.ArgumentNullException: Value cannot be null ConvertToArrayFilter: System.ArgumentNullException: Value cannot be null Nov 12, 2020
@eran-gil
Copy link
Collaborator

You are right, it does work, my bad :)
Anyways, it seems either somehow instead of "Alice" you send null or somehow the value is overwritten to null in log4stash

@DmitryMak
Copy link
Author

You are right, it does work, my bad :)
Anyways, it seems either somehow instead of "Alice" you send null or somehow the value is overwritten to null in log4stash

We remove "Alice" by calling IDisposable.Dispose. I updated the bug description with a code sample.

@eran-gil
Copy link
Collaborator

From what I see, since you dispose "Alice" before logging without popping it from the stack it stays null and therefore causes the exception,
I debugged log4stash with a sample code similar to yours and that's what I saw

@DmitryMak
Copy link
Author

DmitryMak commented Nov 12, 2020

My understanding is that explicit Pop call is not required. Based on log4net documentation for the Push, they seem to recommend this pattern:

using(log4net.LogicalThreadContext.Stacks["NDC"].Push("Stack_Message"))
{
    log.Warn("This should have an ThreadContext Stack message");
}

@eran-gil
Copy link
Collaborator

It was my understanding too, but in fact that's what happens when debugging the code

@DmitryMak
Copy link
Author

I have do a bit more research on this, but IMHO ToArray filter needs to be fixed to behave like ToString filter. ToString does not crash in the same scenario.

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

No branches or pull requests

2 participants