Skip to content

ExceptionlessTarget - ProcessQueueAsync for ExceptionlessClient on Flush#301

Merged
niemyjski merged 2 commits intoexceptionless:mainfrom
snakefoot:main
Apr 21, 2023
Merged

ExceptionlessTarget - ProcessQueueAsync for ExceptionlessClient on Flush#301
niemyjski merged 2 commits intoexceptionless:mainfrom
snakefoot:main

Conversation

@snakefoot
Copy link
Copy Markdown
Contributor

@snakefoot snakefoot commented Apr 20, 2023

Better user-experience when using NLog, and NLog InternalLogger is the recommended place to look when troubleshooting.

@snakefoot
Copy link
Copy Markdown
Contributor Author

snakefoot commented Apr 20, 2023

@niemyjski I can see that #241 changed the default LogLevel from Trace to Warn. Would it make sense that NLog LoggingRule controlled what LogLevel should produce output? (See that Serilog-Sink has similar issues with the new default value)

Example of using minLevel="Info" with NLog.config:

<?xml version="1.0" encoding="utf-8" ?>
<nlog xmlns="http://www.nlog-project.org/schemas/NLog.xsd"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <targets async="true">
    <target type="Exceptionless, Exceptionless.NLog" name="exceptionless" apiKey="API_KEY_HERE">
      <field name="host" layout="${machinename}" />
      <field name="process" layout="${processname}" />
      <field name="user" layout="${environment-user}" />
    </target>
  </targets>

  <rules>
    <logger name="*" minlevel="Info" writeTo="exceptionless" />
  </rules>
</nlog>

Could also consider adding a new GetMinLevel where one could specify the default value for new sources. Where NLog could specify Trace-LogLevel (instead the new default Warn-LogLevel)

}

protected override void FlushAsync(AsyncContinuation asyncContinuation) {
_client.ProcessQueueAsync().ContinueWith(t => asyncContinuation(t.Exception));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is FlushAsync (https://nlog-project.org/documentation/v4.4.0/html/M_NLog_Targets_Target_FlushAsync.htm) called during shutdown or periodically during buffering? If it's while it's buffering, then we probably should remove this as that's already taken care of.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NLog only calls Flush on process-exit, or if the user explict calls NLog.LogManager.Flush() or NLog.LogManager.Shutdown()

Copy link
Copy Markdown
Member

@niemyjski niemyjski 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 PR! Looks great and I'll merge it, left one question.

@niemyjski niemyjski merged commit 9e24a9c into exceptionless:main Apr 21, 2023
@niemyjski
Copy link
Copy Markdown
Member

@snakefoot yeah, I was considering that the logging targets could just set the default min log level on the client (which is only used until server settings were retrieved and last call to that method wins.

@snakefoot
Copy link
Copy Markdown
Contributor Author

Sounds like a sensible way of handling minimum-loglevel, that you perform a "handshake" on first logevent, instead of always using LogLevel.Warn as default. Much more user-friendly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants