fix(logging): ensure callback executes even if Application Insights throws#791
fix(logging): ensure callback executes even if Application Insights throws#791yashkohli88 wants to merge 1 commit into
Conversation
…hrows Wrap Application Insights tracking calls in try/finally block to prevent Winston from hanging if the AI client throws an exception. Without this, a failed trackTrace or trackException call would skip the callback, causing Winston to stall. Addresses feedback from clearlydefined/service#1582
| const err = new Error(info.message) | ||
| err.stack = info.stack | ||
| this.aiClient.trackException({ exception: err, properties }) | ||
| try { |
There was a problem hiding this comment.
No catch here, so a throw from the AI client still escapes through Winston into the caller. The callback fires, but the calling code crashes. Catch it and drop the telemetry quietly (or console.error it). Losing a log line beats killing the request that produced it.
| severity: mapLevel(info.level), | ||
| properties | ||
| }) | ||
| } finally { |
There was a problem hiding this comment.
Needs a catch paired with this. Without one the exception still propagates after callback() runs, so we get the callback guarantee but lose the safety.
JamieMagee
left a comment
JamieMagee
left a comment
There was a problem hiding this comment.
finally makes sure the callback runs, but without a catch the exception from trackTrace / trackException still escapes through Winston into whoever called logger.info(). That swaps the silent hang for a crash on every failed telemetry call. Wrapping in try / catch / finally and dropping (or console.error-ing) the error in the catch fixes it.
Worth adding a test too: stub trackTrace to throw and assert that logger.info() doesn't throw and that the callback still fires. The current suite only hits the happy path, which is why this slipped through.
2 participants
Problem
ApplicationInsightsTransport.log() callback could be skipped if Application Insights client throws exception during trackTrace/trackException calls.
This would cause Winston to hang.
Solution
Wrap AI tracking calls in try/finally block to guarantee callback execution regardless of whether tracking succeeds or fails.
Context
Implements feedback from @JamieMagee on clearlydefined/service#1582 - same issue applies to crawler's ApplicationInsightsTransport implementation
added in #739 #740
Changes