NLog + log4net added NetStandard support - #167
Conversation
|
|
514e768 to
2d5e2b1
Compare
2d5e2b1 to
6043faa
Compare
Sergey Kanzhelev (SergeyKanzhelev)
left a comment
There was a problem hiding this comment.
Thank you for contribution! Please see comments in my review
| </PackageReference> | ||
| <PackageReference Include="Microsoft.ApplicationInsights" Version="2.6.0-beta2" /> | ||
| <PackageReference Include="log4net" Version="2.0.5" /> | ||
| <PackageReference Include="log4net" Version="2.0.8" /> |
There was a problem hiding this comment.
is this update required for the PR? Can the older version reference be kept for better compatibility?
There was a problem hiding this comment.
Oh, I see... older version doesn't have netstandard target.
Can you please enable optional reference here - older version for net45 and newer fro netstandard.
There was a problem hiding this comment.
Now only using ver. 2.0.8 for netstandard, but I have to force log4net from 2.0.5 to 2.0.6 on net45 to support flush.
| <GenerateAssemblyTitleAttribute>false</GenerateAssemblyTitleAttribute> | ||
|
|
||
| <TargetFrameworks>net45</TargetFrameworks> | ||
| <TargetFrameworks>netstandard1.3;net45</TargetFrameworks> |
There was a problem hiding this comment.
for the new target there should be corresponding test project. It will be basically a copy paste of net45. We copying them as Visual Studio test explorer doesn't understand yet multiuple target platfgorms in a single test project.
There was a problem hiding this comment.
Have added netcore-unit-test-projects for NLog and log4net
Sergey Kanzhelev (SergeyKanzhelev)
left a comment
There was a problem hiding this comment.
Thanks again! I'll merge if there will be no more feedback by tomorrow.
There was a problem hiding this comment.
Thanks for contributing!
Can you please update changelog as well with a one liner describing this change/link to GH issue.
https://github.com/Microsoft/ApplicationInsights-dotnet-logging/blob/develop/CHANGELOG.md
|
Cijo Thomas (@cijothomas) Updated changelog.md with a 3 liner |
|
|
||
| IDictionary<string, string> propertyBag; | ||
|
|
||
| if (trace is ExceptionTelemetry) |
There was a problem hiding this comment.
Would it help to use the ISupportProperties interface here? i.e.
propertyBag = ((ISupportProperties)trace).Properties;There was a problem hiding this comment.
I'm just moving code around (stylecop) to fix the unit tests (No PrivateObject in NetCore)
There was a problem hiding this comment.
Ah, I see. Fine to leave it as it is, then.
|
|
||
| if (!string.IsNullOrEmpty(logEvent.LoggerName)) | ||
| { | ||
| propertyBag.Add("LoggerName", logEvent.LoggerName); |
There was a problem hiding this comment.
Consider nameof(logEvent.LoggerName)
There was a problem hiding this comment.
I'm just moving code around (stylecop) to fix the unit tests (No PrivateObject in NetCore)
There was a problem hiding this comment.
Fine to leave it as it is, then.
| ### Version 2.6.0-beta3 | ||
| - [NetStandard Support for NLog and log4net](https://github.com/Microsoft/ApplicationInsights-dotnet-logging/pull/167) | ||
| - [NLog and log4net can Flush](https://github.com/Microsoft/ApplicationInsights-dotnet-logging/pull/167) | ||
| - Update log4net reference to [2.0.6](https://www.nuget.org/packages/log4net/2.0.6) |
There was a problem hiding this comment.
While you're here, please could you also add a line indicating that we also added .Net Standard support for TraceListener:
- [NetStandard Support for TraceListener](https://github.com/Microsoft/ApplicationInsights-dotnet-logging/pull/166)
There was a problem hiding this comment.
Done
|
When will this be released? Thanks in advance! |
|
@MS-TimothyMothra is working on next beta as we speak. Timothy, can you confirm the estimated date of this release? |
|
Earliest will be end of next week (Apr 13th) maybe slipping to early the week of the 16th. This release is dependent on dotnet and dotnet-server. Those are in progress now and should be finished early-mid next week. (Apr 9-11). |
|
Microsoft.ApplicationInsights.NLogTarget ver. 2.6.0-beta3 has been released: https://www.nuget.org/packages/Microsoft.ApplicationInsights.NLogTarget/2.6.0-beta3 |
Updated to NLog ver. 4.5.0 RTM
Updated to log4net ver. 2.0.8 RTM
Added support for Flush()
Resolves #155 + Resolves #68 + Resolves #115