search

datalust / nlog-targets-seq

An NLog target that writes events to Seq. Built for NLog 4.5+.
https://getseq.net
Apache License 2.0
21 stars 11 forks source link

feature/dynamic-log-level #50

Closed NameOfTheDragon closed 4 years ago

NameOfTheDragon commented 4 years ago

Reads the MinimumAcceptedLevel tag from the server HTTP/201 Created response and filters out log events below that level.

Does not alter the NLog configuration.

Level changes are logged to the NLog internal logger.

Bumped package version to 1.3.0 (I wasn't sure whether I should do that, so flagging it up here).

Removed obsolete <PackageIconUrl> because dotnet pack throws an error if it is present. Replaced it with the newer <PackageIcon> element and added the logo image into the package.

Test nuget package is up on my MyGet Community Feed if anyone wants to try it out.

NameOfTheDragon commented 4 years ago

Are you happy to add some tests for updating the level dynamically?

Yes, I think that would be worth doing. I will have a look at it and submit an updated PR.

NameOfTheDragon commented 4 years ago

Are you happy to add some tests for updating the level dynamically?

Yes, I think that would be worth doing. I will have a look at it and submit an updated PR.

I've had a look into providing tests and I'm not sure if the end justifies the means.

The Seq sink tests that you linked to will not map to my solution because the implementations are very different. To test this, I would need to simulate HTTP requests and responses and be able to check that the requests are sent correctly, etc.

Since the request and response processing is encapsulated in a non-testable way, I would have to re-write the entire class to add seams for unit testing. It is not self-evident to me that this doesn't come with a greater risk of introducing problems and/or performance issues than the risk of not having the tests. I would also note that the time required for doing this is likely to be a multiple of the time taken to actually add the implementation.

I am open to persuasion either way and look forward to your thoughts. However, this might be one of those times where you say "working code is the arbiter of success".

NameOfTheDragon commented 4 years ago

I pushed another commit that mops up the cosmetic items. My workload has taken off and unfortunately I'm unable to commit time to writing the tests at present. I have to be honest and say that I think its unlikely that I will return to this. I would have to completely re-write the target class to add seams for testing and to me that is a higher risk than not having the tests, so you might want to consider going ahead without them.

nblumhardt commented 4 years ago

Thanks for this, @NameOfTheDragon - it's a great step forwards; we can take this from here and build on it 👍