feat(Core): set log level using config - #2865
Conversation
|
Important Review skippedToo many files! This PR contains 232 files, which is 82 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (232)
You can disable this status message by setting the No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe naturalized metadata loading path no longer emits diagnostic ChangesWADO-URI logging cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change removes logging from the DICOM image-loading path without introducing a documented behavior or production risk; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@daker is there a particular reason why you are removing these particular loggings? |
|
logging in a hot path can slowdown the rendering, those logs are unskipable |
daker
changed the title
|
@jbocce i replaced most of the console calls to logger and added a new config to let users toggle the logging |
Thanks. Although, I was really only expecting the one file to change 😃, but I think it is fine. @wayfarer3130 are you ok with these changes too? |
|
Please check the failing unit tests. |
|
I'm not sure about the playwright test failures. |
|
@jbocce I fixed the tests, but I'm seeing the same Playwright test failures on other new PRs, so I don't think they're related to my code changes. |
Yes, it looks like this example is broken and thus the related tests are also broken... https://www.cornerstonejs.org/live-examples/contourrenderingconfiguration. |
|
Please update the description to actually match - this PR seems a lot mroe about moving console logs into loglevel logs so that they can be configured. |
|
|
||
| ```ts | ||
| import { log } from '@cornerstonejs/core'; | ||
|
|
There was a problem hiding this comment.
This import doesn't work. @cornerstonejs/core never exports log - it's behind the utilities.logger namespace.
Use one of:
import { logging } from '@cornerstonejs/utils';
logging.log.getLogger('cs3d.dicomImageLoader.wadouri').setLevel('info');or import { utilities } from '@cornerstonejs/core' and then utilities.logger.log.
Same line appears in the two snippets further down.
|
|
||
| // One area | ||
| log.getLogger('cs3d.core.RenderingEngine').setLevel('debug'); | ||
|
|
There was a problem hiding this comment.
Please double check this, but I think this doesn't set the level for an area but instead only for the one logger named exactly cs3d.core.RenderingEngine.
dcmjs pins loglevel 1.9.2, which keeps every logger in one flat map (_loggersByName). Names with dots in them are just strings; there is no parent/child relationship. So cs3d.core.RenderingEngine.StackViewport stays at whatever level it had. Again double check this.
Only the root log.setLevel() reaches everything (it rebuilds all loggers that have no level of their own).
So either reword this to "one logger", and/or add an example that loops over log.getLoggers() and sets every name starting with the prefix.
| ## Logging | ||
|
|
||
| Cornerstone writes its messages with [loglevel](https://github.com/pmurias/loglevel), | ||
| through the same log root as dcmjs. Configure the logs with the loglevel |
There was a problem hiding this comment.
Wrong repo - loglevel is at https://github.com/pimterry/loglevel.
| import { utilities as cornerstoneUtilities } from '@cornerstonejs/core'; | ||
|
|
||
| const cs3dLogger = cornerstoneUtilities.logger.workerLog.getLogger( | ||
| 'polymorphicSegmentation.workers.polySegConverters' |
There was a problem hiding this comment.
workerLog is cs3d.worker, so this name comes out as cs3d.worker.polymorphicSegmentation.workers.polySegConverters.
worker isn't a package, so the package ends up in the third slot instead of the second, which doesn't match the cs3d.<PACKAGE>.<PATH>.<FILEORAREA> spec in the docs. Same in packages/tools/src/workers/computeWorker.js and packages/core/src/webWorkerManager/webWorkerManager.ts (which is core, not a worker package).
Suggestion: drop workerLog and use the package's own logger, so this becomes cs3d.polymorphicSegmentation.workers.polySegConverters. Being in a worker is already visible from the path.
| import { utilities as cornerstoneUtilities } from '@cornerstonejs/core'; | ||
|
|
||
| const cs3dLogger = cornerstoneUtilities.logger.toolsLog.getLogger( | ||
| 'polymorphicSegmentation.canComputeRequestedRepresentation' |
There was a problem hiding this comment.
polymorphic-segmentation is its own package, but using toolsLog names it cs3d.tools.polymorphicSegmentation.*. Someone silencing cs3d.tools would silence this too.
Same in packages/labelmap-interpolation.
Suggestion: add polySegLog / labelmapInterpolationLog next to the other package loggers in packages/utils/src/utilities/logging/index.ts, and list both packages in the docs' <PACKAGE> examples.
| import { utilities as cornerstoneUtilities } from '@cornerstonejs/core'; | ||
|
|
||
| const cs3dLogger = cornerstoneUtilities.logger.cs3dLog.getLogger('adapters.Cornerstone3D.MeasurementReport'); | ||
|
|
There was a problem hiding this comment.
The name is right, but adapters is the only package with no package logger - it uses cs3dLog and writes adapters. into the string by hand. Every other package has one (coreLog, toolsLog, ...).
Suggestion: add adaptersLog and use adaptersLog.getLogger('Cornerstone3D.MeasurementReport'), so the package prefix can't drift.
|
Thanks for working on this! The current console spam is having a significant impact on the DX for applications using the library. I’m seeing several console logs per second during normal series loading, which can quickly result in hundreds of messages flooding the browser console. The main issue for users is that these messages are emitted via I completely agree that standardizing the overall logging architecture is worth discussing, especially given the namespace issues raised in this PR. However, would it be possible to take a phased approach? A minimal fix to suppress/remove the unskippable This would allow us to solve the immediate user-facing problem without blocking the fix on the larger logging architecture discussion. Thanks again to the author and maintainers for working through this! |
|
@daker just following up on this. I think there are review comments from Aug 20 I made that have not been addressed yet - I might be wrong. Are you planning on following up on these? I just don't want this PR that touches many files to go stale because of other commits and/or new files that might have come up since. If we could finish this up soon then that would be great. Thanks so much. |
|
@jbocce yes i will push the fixes |
4 participants
Context
Fixes #2848
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit