NLog ApplicationInsightsTarget Flush with async delay#176
NLog ApplicationInsightsTarget Flush with async delay#176SergeyKanzhelev merged 5 commits intomicrosoft:developfrom
Conversation
|
Thank you for another contribution. We appreciate it. For the housekeeping purposes - can you please create an issue with the problem description and then include changelog update in PR that would refer this issue. |
…to flush (changelog)
|
@SergeyKanzhelev Updated changelog for you. And created issue #177 |
SergeyKanzhelev
left a comment
SergeyKanzhelev
left a comment
There was a problem hiding this comment.
It's a disappointment we need to work around this here, not make it a feature of base SDK (microsoft/ApplicationInsights-dotnet#482). I appreciate your contribution and working around current limitations!
| else | ||
| { | ||
| // Documentation says it is important to wait after flush, else nothing will happen | ||
| // https://docs.microsoft.com/en-us/azure/application-insights/app-insights-api-custom-events-metrics#flushing-data |
There was a problem hiding this comment.
please remove en-US from link so it will direct to localized version of the doc
| { | ||
| // Documentation says it is important to wait after flush, else nothing will happen | ||
| // https://docs.microsoft.com/en-us/azure/application-insights/app-insights-api-custom-events-metrics#flushing-data | ||
| System.Threading.Tasks.Task.Delay(500).ContinueWith((task) => asyncContinuation(null)); |
There was a problem hiding this comment.
Please use TimeSpan.FromMilliseconds(500) to make code a little bit more self-explanatory
|
I'll wait for other people to chime in and will merge tomorrow |
…to flush (code review)
Yes it is a little weird not to have proper flush-logic in a logging framework. |
2 participants
But only when something to flush. Partially resolves #177 for NLog (Probably also an issue for all other logging destinations)