Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add missing handling for LoggingEvent properties #7
Conversation
Occasionally we need properties tied to a specific event and not a ThreadContext which is shared across the entire thread. This hooks into the built in functionality in log4net for this scenario.
|
Thanks this looks like a good addition I just need someone else to test it since we don't have unit tests set up yet |
|
@mostlyjason do you want me to set up a unit test project and add some tests for this PR? |
|
@asser-dk oh wow I wasn't expecting you to volunteer, but it would definitely be appreciated! |
asser-dk
force-pushed the
asser-dk:add-handling-for-logging-event-properties
branch
from
706a4ab
to
37baefd
|
@mostlyjason There we go. I've added some unit tests for the pre parse method in the LogglyFormatter. I also found an issue (I guess it qualifies as that) when writing these tests. Depending on which context you used, if you stored a complex type (like a model object) it would either be serialized as "MyNamespace.Models.MyComplexType" or it would be serialized as a proper json object. My "Harmonize" commit aligns all these cases so complex types are always serialized as json objects. If you want that change in a separate pull request let me know and I'll do that instead :) |
|
Wow this is really awesome! Thanks for making the contribution. Since this is a big change in the first time we are adding unit tests, I'm going to ask our developer to do some manual testing as well. |
…ndling-for-logging-event-properties Conflicts: source/log4net-loggly/LogglyFormatter.cs source/log4net-loggly/log4net-loggly.csproj
|
@mostlyjason do you have an idea of when you'll be able to look at this and hopefully merge+release it? I have some PO's breathing down my neck (and they have very bad breath) |
|
@asser-dk I understand let me talk to my developer and see if we can get this one moved up. I'm hoping she can take a look in the next couple days. |
|
Our engineer tested this and it worked fine. Thanks so much @asser-dk! I'm merging these changes and our engineer will release a new beta version on nuget. |
|
@asser-dk we published the new package can you test this one? https://www.nuget.org/packages/log4net-loggly/7.3.2-beta |
|
@mostlyjason sure. I'll have a look at it later today. |
|
@mostlyjason There seems to be an issue with the nuget package. It doesn't contain the lib folder (or anything for that matter besides the .nupkg file). I downloaded the 7.2.3 version which contains this. log4net-loggly.7.3.2-beta package contents: log4net-loggly.7.2.3 package contents: |
|
@Shwetajain148 can you take a look at the package to make sure it was built correctly? |
|
works :) |
|
Great thanks we will work on releasing a new production version |
Occasionally we need properties tied to a specific event and not a ThreadContext which is shared across the entire thread. This hooks into the built in functionality in log4net for this scenario.