Fixed errors not logging to SelfLog #284
Conversation
| @@ -72,7 +72,7 @@ protected override void EmitBatch(IEnumerable<LogEvent> events) | |||
| var items = result.Body["items"]; | |||
| foreach (var item in items) | |||
| { | |||
| if (item["index"] != null && HasProperty(item["index"], "error") && item["index"]["error"] != null) | |||
There was a problem hiding this comment.
The HasProperty function is not used now anymore. Would it be possible to include the ContainsKey functionality inside the HasProperty function instead? In order to make that one more robust?
Is there any way to test this BTW? So does it not break now for the other versions of EF?
There was a problem hiding this comment.
Agreed. It should suffice to change the first condition of HasProperty from if (settings is System.Dynamic.ExpandoObject) to if (settings is System.Collections.IDictionary || settings is System.Dynamic.ExpandoObject).
I believe item isn't always an IDictionary, in fact I think I tried .ContainsKey first and it didn't work for me because it was some other kind of dynamic object.
No idea about how to integration test this, unfortunately. Unit testing the HasProperty function would be trivial of course but wouldn't help at all with ES version changes.
|
@TooLazyToMakeAName can't wait to have this corrected and merged - thank you. |
|
I too am affected by this issue and would love to see this fix get through. @mivano if @TooLazyToMakeAName is MIA for much longer I am willing to take this over and push it across the finish line. |
|
Please do, I m not using the sink itself, so anybody that can provide some help to indicate that this is a needed change is more than welcome. |
|
Any progress? |
|
Sorry, nothing from me. Not actively using the sink at the moment. If someone can pick this up and validate that this is working as desired, we can merge it in. |
|
I think with another PR, this might not be needed anymore? |
|
Too much drift, other changes got in as well. So closing this one. |
What issue does this PR address?
Elasticsearch error messages not logged to selflog. #280
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements