fix(sdk-logs): stop Logger.emit() doing work after LoggerProvider shutdown#6826
Conversation
…tdown emit() previously built a LogRecord, reported logger metrics and called processor onEmit even after the owning LoggerProvider had shut down. Track shutdown state on LoggerProviderSharedState, set it in LoggerProvider shutdown, and short-circuit both emit() and enabled() so existing Loggers do no work once the provider is shut down (matching the OpenTelemetry Java SDK).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6826 +/- ##
=======================================
Coverage 95.56% 95.56%
=======================================
Files 386 386
Lines 13084 13088 +4
Branches 2997 2998 +1
=======================================
+ Hits 12504 12508 +4
Misses 580 580
🚀 New features to boost your workflow:
|
trentm
left a comment
There was a problem hiding this comment.
Thanks! LGTM.
I have a few nits, mostly on personal preferences on comment verbosity.
|
Thanks @trentm — fair points. On the redundant For the comment verbosity: happy to trim the guards-with-explanation style if the preference is to let the guards speak for themselves. I'll hold off on pushing the cleanup until a core team member weighs in — don't want to churn the commit unnecessarily if the preference is to land as-is. But if you'd like me to update, just say the word. |
|
@anneheartrecord - Trent is the core team member (he's a maintainer) 😄 I also agree that we should reduce the verbosity of the comments a bit. I think the code is very readable as-is :) |
…uard and trim verbose comments - Remove early `hasShutdown` short-circuit from `emit()`; the subsequent `enabled()` call already gates on shutdown, so the guard was redundant - Drop two-line explanatory comments before guards that speak for themselves in Logger.ts, LoggerProvider.ts, and LoggerProviderSharedState.ts Suggested by trentm and seconded by pichlermarc in code review.
|
Done — pushed the cleanup:
Tests still pass, lint clean. |
ec5aa93
|
Thank you for your contribution @anneheartrecord! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Which problem is this PR solving?
Fixes #6823
Logger.emit()only guards onthis.enabled(), which covers severity / trace-based / per-processor filtering but not provider lifecycle. Once the owningLoggerProviderhas been shut down, an already-obtainedLoggerstill builds aLogRecordImpl, reports logger metrics and callsactiveProcessor.onEmit()— exactly the side work raised in the SIG review. The shutdown state lived only inLoggerProvider._shutdownOnce, which theLogger(holding onlyLoggerProviderSharedState) could not see.Short description of the changes
LoggerProviderSharedState(hasShutdown) and set it inLoggerProvidershutdown, so it also covers emits that race with the in-flight processor shutdown.Logger.emit()when shut down — no record construction, no metrics, noonEmit.Logger.enabled()returnfalseafter shutdown as well, so callers that useenabled()to gate expensive log construction don't waste that work (the API documentsenabled()for exactly this purpose).This mirrors the
loggerSharedState.hasBeenShutdown()guard in the OpenTelemetry Java SDK referenced in the issue.How Has This Been Tested?
Added a regression test in
Logger.test.ts: afterawait loggerProvider.shutdown(),logger.enabled()returnsfalseandlogger.emit()neither calls the processor'sonEmitnor constructs aLogRecord. The fullsdk-logssuite passes (200 passing).