Closed Punzo closed 5 months ago
Some suggestions (wishlist):
Some suggestions (wishlist):
@lassoan thanks for the feedback. I will apply them on Monday (tomorrow I am off). The implementation should be straightforward.
Fix also: Excessive logging in debug mode (from @lassoan).
Logging currently is at two levels:
1) DCMTK: it is a static level which can be changed at runtime. By default the the ctkDICOMScheduler set it to warning
in its creator
Slicer set it at run time with the settings (detailed logging in the DICOM settings): https://github.com/Slicer/Slicer/pull/7525/files#diff-522c86f7cb8b2b77df9d521eca44263ad00536c63f25182df3d75128dd09a16aR422-R427
2) CTK: each class can have static logger one, for example:
https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L51 https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L466
these logger classes have the logging level set at compilation as: https://github.com/commontk/CTK/blob/master/Libs/Core/ctkLogger.cpp#L34-L38
I guess Andras issue is regarding (2). A fast fix could be setting warning
or info
level (instead of debug
) if the project is set in debug configuration (in release mode currently is warning
). I better fix could be to allow to change it at run time and have a slicer configuration in the settings.
Fix also: Excessive logging in debug mode (from @lassoan).
Logging currently is at two levels:
- DCMTK: it is a static level which can be changed at runtime. By default the the ctkDICOMScheduler set it to
warning
in its creatorSlicer set it at run time with the settings (detailed logging in the DICOM settings): https://github.com/Slicer/Slicer/pull/7525/files#diff-522c86f7cb8b2b77df9d521eca44263ad00536c63f25182df3d75128dd09a16aR422-R427
- CTK: each class can have static logger one, for example:
https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L51 https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L466
these logger classes have the logging level set at compilation as: https://github.com/commontk/CTK/blob/master/Libs/Core/ctkLogger.cpp#L34-L38
I guess Andras issue is regarding (2). A fast fix could be setting
warning
orinfo
level (instead ofdebug
) if the project is set in debug configuration (in release mode currently iswarning
). I better fix could be to allow to change it at run time and have a slicer configuration in the settings.
@lassoan for the moment I have simply set the ctk LogLevel
to Info
for debug mode and Warning
for release mode. In CTK logger.debug
was the most used one. This highly reduces the number of logging outputs. Developers can always change it back locally. See commit:
https://github.com/commontk/CTK/pull/1184/commits/3739cde0cb6121a653fe6b0f1b7a394ae80c23c5
Moving forward, we either:
1) implement a proper logger pattern (e.g. DCMTK logger), where we could set the level globally at run time.
2) we move to use QLoggingCategory
(https://doc.qt.io/qt-5/qloggingcategory.html) as already proposed by @jcfr ( see https://github.com/commontk/CTK/pull/1181#issuecomment-1895602751)
[x] (1) Add column for job creation date/time.
[x] (2) Add progress column (% completion)
[x] (3) Add patient name as column (and maybe birth date and modality as well if easy to obtain)
[x] (4) Make column names human-readable (with spaces) and translatable
[x] (5) Study instance UID, series instance UID, SOP instance UID, Job UID do not contain directly useful information for the user and they take up a lot of space. I would even remove the DICOMLevel column and just merge it with the job class (as described above). Instead of polluting the table with these columns (making the table more complex, larger, and leaving less space for relevant information), it could be better to have a collapsible "Details" section (or popup) with a textbox that contains all the details of the job, including status, errors, etc. in plain text that can be easily copy-pasted for troubleshooting. If multiple jobs are selected then the details text would contain the concatenation of the information from all jobs.
[ ] (6) find a way to log DCMTK responses per job. i.e. instead of priting DCMTK logging to terminal, we need to have a stream for each Job (identified by the JobUID) to the ctkDICOMJobListWidget (which will be printed in the details)
[x] (7) Record the job creation time, when execution started, and when it was completed (and display it in the job details text).
[x] (8) Filter is usually at the top (see the old DICOM browser, the application log, etc.), but since we have some buttons on the right, I think we could have filtering options on the right. Maybe something similar to how the Windows event log works (although there filtering settings are in a popup, which is not very user friendly):
[x] (9) It is not clear how "Show all" relates to filtering. It would be better to have all filtering options at the same place. Instead of "Show all" probably "Show completed" (turned off by default) would be more clear, and maybe we can add a "Reset" button next to the filter (that resets to the default filtering settings, i.e., show all tasks that are not completed).
[x] (10) It is not clear if "Clear" means that it just removes successfully completed jobs, or also failed jobs, or it even stops all in-progress jobs and clears all scheduled jobs. Maybe a broom icon and change to "Clear completed" caption would make it clear.
We should have a quick way of retrying all failed jobs (1-2 button clicks)
My issue (https://github.com/commontk/CTK/issues/1186) with logging for now is with logger.debug( "SQL worked!\n SQL: " + query.lastQuery());
code in ctkDICOMDatabase.cpp. Such messages are logged fairly frequently in cases when everything is fine. So, it is not a debug level message, but a trace level message. If setting to trace level fixes the superflous logging then it could be OK, but for performance reasons I would consider enabling them with #ifdef.
My issue (#1186) with logging for now is with
logger.debug( "SQL worked!\n SQL: " + query.lastQuery());
code in ctkDICOMDatabase.cpp. Such messages are logged fairly frequently in cases when everything is fine. So, it is not a debug level message, but a trace level message. If setting to trace level fixes the superflous logging then it could be OK, but for performance reasons I would consider enabling them with #ifdef.
I agree. I have set the majority of the loging from the database class as trace
and switched back to debug
the ctk log level for debug mode. https://github.com/commontk/CTK/pull/1184/commits/b65c0f626c020804375ea51474ad5d74b73d878f
Debug Logging discussion moved to https://github.com/commontk/CTK/issues/1186 and https://github.com/commontk/CTK/pull/1187
regarding this PR @lassoan I need only to finish (2 - progress bars) and (6 - stream DCMTK logging/errors per each job to the UI) from https://github.com/commontk/CTK/pull/1184#issuecomment-1903724977 ETA: (2) tomorrow (6) asap.
@lassoan (2) progress bars done
See video:
https://github.com/commontk/CTK/assets/7985338/831c8c44-2694-4ab3-afc4-3a085cf4dc12
NOTE: I also have fixed: 1) some issues with CMOVE when using the new ctkDICOMStoregeListener 2) perfomances improvements: in some cases there were unnecessary re-renderings of the thumbnails.
I will try to have (6 - stream DCMTK logging/errors per each job to the UI) asap.
But I would consider to merge this PR this week, without (6). This PR is already very large and streaming the logging to the UI will be another large commit probably. It would be ideal to have this in Slicer before the project week. Please let me know if you agree @lassoan @jcfr
(6 - stream DCMTK logging/errors per each job to the UI): this can be implemented by instantiating an Appender
with a custom ErrorHandler
to the SCU logger:
DCM_dcmnetLogger (name: "dcmtk.dcmnet")
@lassoan @jcfr this will be done in a different PR (most likely next week at the project week) .