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

LoggingConfigurationParser - Added support for using assembly-name in type-name #4300

Merged
merged 2 commits into from
Feb 14, 2021

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Feb 13, 2021

Resolves #2944 by allowing you to do this:

<targets>
   <target name="evt" type="NLog.WindowsEventLog.EventLog" />
</targets>

Making it easier to specify types from NLog-extension-assemblies without also having to update <extensions>

Update #4308 Now changed it into this:

<targets>
   <target name="evt" type="EventLog, NLog.WindowsEventLog" />
</targets>

@snakefoot snakefoot added this to the 5.0 (new) milestone Feb 13, 2021
@snakefoot snakefoot force-pushed the ExtensionTypeLoadAssembly branch 3 times, most recently from e4e668c to cdbe308 Compare February 13, 2021 23:18
@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 13, 2021

@ThomasArdal Notice NLog will now attempt to extract Extension-Assembly-Name from the type-prefix.

This means NLog will attempt to load assembly-name "elmah", when having forgotten to include Elmah.Io.NLog in <extensions>:

<targets>
  <target name="elmahio" type="elmah.io" apiKey="API_KEY" logId="LOG_ID"/>
</targets>

And if one tries to make use of this feature with Elmah.io, then it fail to recognize that "elmah.io" is the symbol-name. But instead think that "io" is the symbol-name and "Elmah.Io.NLog.elmah" is the assembly-name:

<targets>
  <target name="elmahio" type="Elmah.Io.NLog.elmah.io" apiKey="API_KEY" logId="LOG_ID"/>
</targets>

Btw. funny the Elmah.io with NLog has no examples or documentation of how to setup NLog. Maybe include a link to the Docs (or nuget-package)

@snakefoot snakefoot force-pushed the ExtensionTypeLoadAssembly branch 2 times, most recently from 21f8e9b to ecd9173 Compare February 14, 2021 00:09
@ThomasArdal
Copy link

@snakefoot If I remember corretly, the elmah.io type references this, right?

[Target("elmah.io")]

Would it change anything if the target is named elmahio or elmah-io? I guess having a dot in the name is bad for other reasons too.

@snakefoot
Copy link
Contributor Author

@ThomasArdal Yes instead of this:

    [Target("elmah.io")]
    public class ElmahIoTarget : AsyncTaskTarget

Then with NLog 5.0 it is possible to do this:

    [Target("elmah.io")]
    [Target("elmah-io")]
    [Target("ElmahIo")]
    public class ElmahIoTarget : AsyncTaskTarget

@ThomasArdal
Copy link

@snakefoot That's nice. Will add that in time when we move the dependency to 5.0 for sure.

Maybe you should consider looking for exact target name matches before falling back to treating dots as part of an assembly name? This will break compatibility with all targets containing dots in the name. Being a major version change this is valid of course. But could still cause some confusion.

@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 14, 2021

Maybe you should consider looking for exact target name matches before falling back to treating dots as part of an assembly name?

The extension-loading-magic is only done when NLog detects the Type-Symbol-Name contains a dot .. And before activating this magic-loading then NLog first checks if the Type-Symbol-Name is already registered. This means the following example will continue to work without any changes:

<extensions>
  <add assembly="Elmah.Io.NLog"/>  <!-- Registers elmah.io as type-symbol-name -->
</extensions>

<targets>
  <target name="elmahio" type="elmah.io" apiKey="API_KEY" logId="LOG_ID"/>
</targets>

@sonarcloud
Copy link

sonarcloud bot commented Feb 14, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

92.5% 92.5% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit c15241f into NLog:dev Feb 14, 2021
@ThomasArdal
Copy link

@snakefoot Makes sense 👍 I think we'll go for a name change, not including the dot. Thank you for the heads up.

@ThomasArdal
Copy link

@snakefoot I have tested the elmah.io target with NLog 5 (dev branch) and written some more documentation. I just want to provide a few ideas that you can take or leave, but at least I have written down my thoughts somewhere.

I totally understand why you want to support fully qualified references of targets, since adding assemblies in extensions is a common source for confused users and non-working config. But I'm not sure I understand the "schema" you have chosen for the fully qualified name. As I understand it, you will need to reference targets on the following format:

assemblyname.targetname

Where a real example could be:

Custom.NLog.Targets.FancyFile

This will require a target implementation in an assembly named Custom.NLog.Targets with the target attribute and the name of FancyFile.

I think that this may be misleading for some users since this isn't a fully qualified class name. I could see something like this working:

Custom.NLog.Targets.FancyFileTarget

This would reference the file itself. And for target names it could be:

Custom.NLog.Targets, fancyfile

This includes the assembly name containing the target as well as the target name separated with a comma.

The purpose of this comment is not to force you to support:

Elmah.Io.NLog, elmah.io

This would be possible of course, but it's no problem for us to add more Target attributes once we upgrade the minimum version of NLog to v5.

Anyway, that is just my thoughts.

@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 15, 2021

I was thinking that NLog-users standing outside the source-code could easily see 2 things:

  • The symbolic-type-name
  • The nuget-package-name

So I guess that I could change from dottet format to comma-format, and support both:

  • elmah.io, Elmah.Io.NLog
  • ElmahIoTarget, Elmah.Io.NLog

Then instead of scanning for dot . then it will scan for comma , and use that as seperator. But I kept away from actual fully-qualified-names as I find them very wordy with full namespaces etc.

@ThomasArdal
Copy link

Agree. Real fully-qualified names are probably not preferred. I just think that it's a bit hard to see where the assembly/nuget name ends and where the target name starts with the current solution. The last dot, I know, but could be clearer with the comma for users not into the details.

Maybe you should get input from at least one other person that doesn't have a target with a dot in the name before making any changes 😄

@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 15, 2021

@ThomasArdal Maybe you should get input from at least one other person that doesn't have a target with a dot in the name before making any changes

I kind of like having the assembly-name at the end, since the symblic-name is the most important part. So it should be first, since a nuget-package can contain multiple NLog-extensions.

@ThomasArdal
Copy link

Yep agree. I think I had forgotten about the way to reference class names when writing the proposal. targetname, assemblyname is definitely better and follow how it's done in other places in .NET.

@snakefoot
Copy link
Contributor Author

snakefoot commented Feb 15, 2021

@ThomasArdal Thank you for the feedback, much appreciated. Have now created #4308

@snakefoot
Copy link
Contributor Author

Updated wiki: https://github.com/NLog/NLog/wiki/Register-your-custom-component about how to use fully qualified name.

@snakefoot snakefoot added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Nov 27, 2021
@snakefoot
Copy link
Contributor Author

It seems that using Elmah.Io.NLog, elmah.io in xsi:type is not valid format, when using XML-schema-validation.

It complains that , (Hexidecimal value 0x2C) is not acceptable. See also #4978

@ThomasArdal
Copy link

But it works on runtime, right? My memory around this is a bit vague but having a fully qualified name there instead of the add assembly extension should be possible in NLog 5. So, maybe the XSD for the xsi:type attribute inside the target element should be updated?

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 18, 2022

xsi:type (And QName) is not under NLog-control, since part of the XML-standard.

But yes NLog uses the format just fine, even though the XML Schema validation fails.

@ThomasArdal
Copy link

Oh, yeah of course. xsi namespace. I see that I am using a type attribute in the documentation but xsi:type in the samples. What would be the better option? type I guess to get the schema validation right.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 18, 2022

When using xsi:type then it allows intellisense and validation of config-options by using NLog.xsd-schema (Can lookup the options for the specified type). Can even get build-warnings when NLog.config XML-file is using unknown options.

@ThomasArdal
Copy link

Yeah ok. I see that IntelliSense no longer works in the sample. I think we have some URLs to update. Like www to non-www and https).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) feature needs documentation on wiki nlog-configuration size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend NLog target extension type-name loading
2 participants