Switch all error and exception logger calls to warning if they do not lead to a crash.

[drew2a]

Currently, we use error, exception, and warning logger calls in Tribler without any established system.

I propose the following approach:

1. Use warnings for all errors that do not lead to a Tribler crash.
2. Use error and exception logs for all critical errors that may lead to a Tribler crash.

The lack of a systematic approach to using these log levels complicates the investigation of bugs and leads to false error detection for CoreCrashedError. When Sentry parses the core output, it identifies the closest logged ERROR, assuming it's the root cause of the CoreCrashedError (see example #7855). However, CoreCrashedError itself often doesn't provide much insight into the cause.

If we agree on this approach, I can perform the switch (estimated time: 2 hours).

[kozlovsky]

To me, a warning is something that can be ignored for a while, and we should not ignore serious Core errors (at least in development) even if those errors do not lead to an immediate Core crash.

Also, parsing the last Core output to extract the reason for the last crash looks like a hack. Even if we change the level of all errors that do not lead to the Core crash to warnings, it still looks possible that a critical error causes a cascade of other critical errors, and the last error in the Core output will still not be the initial error that caused the crash.

My concern is that by switching all non-crashing errors to warnings, we start ignoring them more in development.

Instead, we can extend #7699 in the follow-up PR and provide a robust way to send Core errors to GUI not via the stderr output but via files. Then, in case of the Core crash, we can be sure that the actual error leading to the last Core crash is stored in the last error file, and there should no longer be ambiguity.

Saying this, I'm not against changing some specific non-critical errors to warnings.

[xoriole]

In general, if there is an exception or an error, I tend to use the same log level. The reason being, at the point of logging, the error or exception may or may not be clear whether it crashes (or should crash) the application. The exception may be re-raised again after logging as well.

Switch all error and exception logger calls to warning if they do not lead to a crash

Instead of all error or exception logger calls, I would go for case-by-case basis wherever it makes sense.

Saying this, I'm not against changing some specific non-critical errors to warnings.

I agree with @kozlovsky here.

[drew2a]

Here are my first three results from searching through the codebase:

tribler/src/tribler/core/components/content_discovery/community/content_discovery_community.py

Lines 360 to 372 in 1c6baad

	async def _on_remote_select_basic(self, peer, request_payload, force_eva_response=False):
	try:
	sanitized_parameters = self.parse_parameters(request_payload.json)
	# Drop selects with deprecated queries
	if any(param in sanitized_parameters for param in self.composition.deprecated_parameters):
	self.logger.warning(f"Remote select with deprecated parameters: {sanitized_parameters}")
	self.ez_send(peer, SelectResponsePayload(request_payload.id, LZ4_EMPTY_ARCHIVE))
	return
	db_results = await self.process_rpc_query_rate_limited(sanitized_parameters)
	self.send_db_results(peer, request_payload.id, db_results, force_eva_response)
	except (OperationalError, TypeError, ValueError) as error:
	self.logger.error(f"Remote select. The error occurred: {error}")

tribler/src/tribler/core/components/tunnel/community/tunnel_community.py

Lines 96 to 102 in 1c6baad

	async def _poll_download_manager(self):
	# This must run in all circumstances, so catch all exceptions
	try:
	dl_states = self.download_manager.get_last_download_states()
	self.monitor_downloads(dl_states)
	except Exception as e: # pylint: disable=broad-except
	self.logger.error("Error on polling Download Manager: %s", e)

tribler/src/tribler/core/components/tunnel/community/tunnel_community.py

Lines 301 to 306 in 1c6baad

	def on_e2e_finished(self, address, info_hash):
	dl = self.get_download(info_hash)
	if dl:
	dl.add_peer(address)
	else:
	self.logger.error('Could not find download for adding hidden services peer %s:%d!', *address)

Are these serious errors, or just warnings?

[kozlovsky]

I can't comment on the last one, but the first two look like errors. We should investigate these errors and fix the reason or improve the error handling by making it less broad and generic.

[drew2a]

@kozlovsky @xoriole, thank you for sharing your opinions. Since it is now clear that there is no support for the suggested improvement, I am closing the issue.