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.

Sign up for GitHub

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

Jump to bottom

NLog config file loading: use process name (e.g. applicationname.exe.nlog) when app.config is not available #3261

Merged
merged 1 commit into from
Apr 7, 2019
Merged

NLog config file loading: use process name (e.g. applicationname.exe.nlog) when app.config is not available #3261

merged 1 commit into from
Apr 7, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Mar 30, 2019 •
edited

Trying to resolve #3156 for NetCore

@snakefoot snakefoot force-pushed the NetCoreNLogConfigLoading branch 6 times, most recently from f1d2f29 to 2cd7e4a Compare March 30, 2019 17:47
@codecov
Copy link

codecov bot commented Mar 30, 2019 •
edited

Codecov Report

Merging #3261 into dev will increase coverage by <1%.
The diff coverage is 73%.

@@          Coverage Diff           @@
##             dev   #3261    +/-   ##
======================================
+ Coverage     80%     80%   +<1%     
======================================
  Files        356     356            
  Lines      28007   28016     +9     
  Branches    3728    3728            
======================================
+ Hits       22408   22438    +30     
+ Misses      4520    4501    -19     
+ Partials    1079    1077     -2

@snakefoot snakefoot force-pushed the NetCoreNLogConfigLoading branch 6 times, most recently from 0bec3dc to fb51337 Compare March 30, 2019 20:32
@304NotModified 304NotModified self-requested a review March 30, 2019 21:34
@304NotModified 304NotModified self-assigned this Mar 31, 2019
304NotModified
304NotModified reviewed Mar 31, 2019
View reviewed changes
src/NLog/Internal/ProcessIDHelper.cs Outdated
/// <summary>
/// Initializes the ThreadIDHelper class.
/// </summary>
private static ProcessIDHelper CreateInstance()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 much better.

FYI: I think "instance" in redundant, and most "factory methods" are called Create - so renamed it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think the whole instance thing should be removed. But not sure about the reason for the Win32-native-call. Since the values are cached, then I don't understand remark about Win32 is "without the overhead".

src/NLog/Config/LoggingConfigurationFileLoader.cs Outdated Show resolved Hide resolved
@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 31, 2019 •
edited

Actually think that Win32ProcessIDHelper could be thrown away. Think the original code was this (Causing overhead by allocating Process-object on every lookup.

Process.GetCurrentProcess().Id

Without any caching. Causing it to be much slower than this code:

NativeMethods.GetCurrentProcessId()

But later caching was added so there no overhead at all for either method. Could be nice to remove some of those Win32-native-methods.

@304NotModified
Copy link
Member

Could be nice to remove some of those Win32-native-methods.

Agree on that! Less code is better

@304NotModified 304NotModified removed their assignment Apr 1, 2019
@snakefoot snakefoot force-pushed the NetCoreNLogConfigLoading branch 3 times, most recently from e5fe536 to 30c8d63 Compare April 2, 2019 17:07
@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 2, 2019 •
edited

@snakefoot Have squashed the commits, and tried to ensure correct rename operations of the files.

@304NotModified 304NotModified added this to the 4.6.3 milestone Apr 2, 2019
304NotModified
304NotModified requested changes Apr 3, 2019
View reviewed changes
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

I've a bit s problem with the no-so-lean interface. I know it's only internal, but still it's a degrade of the current interface (IFile). IMO it should be multiple interfaces which has clear separations (Single responsibility principle).

It looks like a lot of code has been moved? It's that really preferred? (Makes review and history more difficult too track)

src/NLog/Config/LoggingConfigurationFileLoader.cs Show resolved Hide resolved
tests/NLog.UnitTests/ConfigFileLocatorTests.cs Show resolved Hide resolved
src/NLog/Internal/Fakeables/IAppEnvironment.cs Outdated Show resolved Hide resolved
@snakefoot snakefoot force-pushed the NetCoreNLogConfigLoading branch 3 times, most recently from f633c0e to d557d53 Compare April 4, 2019 19:23
@304NotModified
Copy link
Member

maybe we should update the docs - and this unfortunately related: #959

@snakefoot
Copy link
Contributor Author

@304NotModified maybe we should update the docs

Well this PR actually tries to ensure that NetCore follows the Wiki :)

@304NotModified
Copy link
Member

so supersedes #959? Isn't this a breaking behavior change?

@snakefoot
Copy link
Contributor Author

@304NotModified so supersedes #959? Isn't this a breaking behavior change?

No and No.

#959 actually implements the config loading so it happens in the correct order. It should always use the application-specific process.exe.nlog instead of a random Nlog.config. But this is a breaking change, so you have stalled this fix for NLog 5.0

This PR only fixes NetCore so it behaves like NetFramework. Because AppDomain.SetupInformation.ConfigurationFile is blank on NetCore-platform and needs special help.

@304NotModified
Copy link
Member

Ok thanks for explaining!

FYI, I like to start with merging 5.0 PRs after 4.6.3 is done.

@snakefoot
Copy link
Contributor Author

FYI, I like to start with merging 5.0 PRs after 4.6.3 is done.

Think we should keep dev and master available for more hotfixes a few more weeks before starting up on NLog 5.0. But your decision.

@304NotModified
Copy link
Member

304NotModified commented Apr 5, 2019 •
edited

Hotfixes could be merged to master then :)

(But features will be 5.0 instead of 4.6.*)

@snakefoot
Copy link
Contributor Author

@304NotModified Is the merge of this PR blocked by something?

@304NotModified
Copy link
Member

304NotModified commented Apr 7, 2019 •
edited

Yes my free time. I try to do some developing next to reviewing. Only reviewing is boring and I'm losing some touch to NLog in that way.

304NotModified
304NotModified approved these changes Apr 7, 2019
View reviewed changes
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

src/NLog/Config/XmlLoggingConfiguration.cs Show resolved Hide resolved
src/NLog/Internal/Fakeables/IFileSystem.cs
/// </summary>
internal interface IFile
internal interface IFileSystem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this was not the direction I would take. I think it's better to have 10 small interfaces then 3 medium.

Also, with the introduction of NSubstitute, really small interfaces are nice.

But as it's internal I think this is more than good enough 👍

@304NotModified
Copy link
Member

@304NotModified Is the merge of this PR blocked by something?

Anyway, pushing has some results ;)

@304NotModified
Copy link
Member

nice results!

image

@304NotModified 304NotModified changed the title NLog config file loading using process name when app.config not available NLog config file loading: use process name when app.config not available Apr 7, 2019
@304NotModified 304NotModified changed the title NLog config file loading: use process name when app.config not available NLog config file loading: use process name (e.g. applicationname.exe.nlog) when app.config not available Apr 7, 2019
@304NotModified 304NotModified changed the title NLog config file loading: use process name (e.g. applicationname.exe.nlog) when app.config not available NLog config file loading: use process name (e.g. applicationname.exe.nlog) when app.config is not available Apr 7, 2019
@304NotModified 304NotModified merged commit 69c0854 into NLog:dev Apr 7, 2019
@snakefoot
Copy link
Contributor Author

@304NotModified Yes my free time. I try to do some developing next to reviewing. Only reviewing is boring and I'm losing some touch to NLog in that way.

Very understandable. I also prefer developing over reviewing :) Will try and reduce the number of PRs for the NLog-project, so everything is not about my PRs :)

But thank you for the merge. Now there is some cleanup to do:

  • Refactor the remaining unit tests in ConfigFileLocatorTests.cs to use the new mock-interface instead of building exe-file (Faster and can be tested on NetCore).
  • Update Load order precedence fix for config files issue #223 #959 to handle the moving of code from LogFactory to LoggingConfigurationFileLoader

I don't mind fixing these two, unless they should be up-for-grabs?

@304NotModified 304NotModified self-assigned this Apr 7, 2019
@snakefoot snakefoot deleted the NetCoreNLogConfigLoading branch April 4, 2020 12:58
@jsm174 jsm174 mentioned this pull request Jan 4, 2021
Closed
This was referenced Jan 4, 2021
Closed
Closed
Merged
@snakefoot snakefoot mentioned this pull request Mar 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified approved these changes

Assignees

@304NotModified 304NotModified

Labels
feature
Projects
None yet
Milestone
4.6.3
Development

Successfully merging this pull request may close these issues.

Configuration in applicationname.exe.nlog files do not load
2 participants
@snakefoot @304NotModified