Reduce log level of common warnings #1029
Conversation
Log levels of warning and above should indicate unexpected behavior to the user. If the application is performing as expected, users shouldn't be misguided by log messages that are not relevant to potential issues. If an issue is encountered because of these warnings without any other relevant details in the log, then the log level can still be increased to get a full debug log.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1029 +/- ##
==========================================
- Coverage 23.83% 23.73% -0.10%
==========================================
Files 137 137
Lines 21873 21986 +113
==========================================
+ Hits 5213 5219 +6
- Misses 16660 16767 +107
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| @@ -570,7 +570,7 @@ impl AtomicDrmSurface { | |||
| } else { | |||
| if current.mode != pending.mode { | |||
| if let Err(err) = self.fd.destroy_property_blob(current.blob.into()) { | |||
| warn!("Failed to destory old mode property blob: {}", err); | |||
| debug!("Failed to destory old mode property blob: {}", err); | |||
There was a problem hiding this comment.
Well this error only occurs at launch, because the current_mode is not an Option, but initialized with a bogus null-blob (which obviously fails to be destroyed). Any further occurrences would indicate an actual error.
However without making it an Option and littering the code with unwraps, I don't see a good way to mitigate that first log message.
There was a problem hiding this comment.
Wouldn't it be possible to just check for Unknown(0) or to have a flag on current.blob which stores if the blob has been initialized or not?
Attempting this operation when it is know that it will fail seems like it's not ideal anyway, so maybe we can just skip the destruction entirely?
| @@ -69,7 +69,7 @@ impl DrmDeviceFd { | |||
| // This is only needed on older kernels. Newer kernels grant this permission, | |||
| // if no other process is already the *master*. So we skip over this error. | |||
| if dev.acquire_master_lock().is_err() { | |||
| warn!("Unable to become drm master, assuming unprivileged mode"); | |||
| debug!("Unable to become drm master, assuming unprivileged mode"); | |||
There was a problem hiding this comment.
yeah, this needs to be a really old kernel version to actually indicate an error. I guess this is fine as a debug statement.
Log levels of warning and above should indicate unexpected behavior to the user. If the application is performing as expected, users shouldn't be misguided by log messages that are not relevant to potential issues.
If an issue is encountered because of these warnings without any other relevant details in the log, then the log level can still be increased to get a full debug log.
From my understanding of what @Drakulix said on IRC, these messages are expected and I cannot change the behavior to avoid their emission. As such I'd rather not have them visible on my compositor's default log level.
I've already excluded Smithay's info messages from my default log level since these are very verbose, but I think that should be fine. I'd really like to be able to emit Smithay warnings by default. If this patch isn't appropriate, I think my only alternative would be exclusively logging errors.