SCANPY-135 Add log messages through the code by thomas-serre-sonarsource · Pull Request #175 · SonarSource/sonar-scanner-python

thomas-serre-sonarsource · 2025-03-31T13:20:22Z

No description provided.

Copilot

Pull Request Overview

This pull request (SCANPY-135) introduces additional logging throughout the codebase to improve traceability of operations and errors. The key changes include:

Adding logging statements in critical functions across the application, including engine provisioning, configuration loading, API operations, and CLI startup.
Updating unit tests to verify the newly added log messages using mocks.
Adjusting import statements to support the new logging integrations.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/unit/test_pyproject_toml.py	Added patching of app_logging.get_logger to verify proper warning logging for malformed TOML.
tests/unit/test_configuration_loader.py	Updated import to include MagicMock alongside patch for enhanced test assertions.
tests/test_environment_variables.py	Patched the logger and updated test for invalid JSON logging, ensuring JSONDecodeError handling.
src/pysonar_scanner/scannerengine.py	Inserted logging messages to trace the engine download and command execution steps.
src/pysonar_scanner/configuration/pyproject_toml.py	Modified the exception handling to log errors during TOML parsing with detailed exception info.
src/pysonar_scanner/configuration/environment_variables.py	Enhanced JSONDecodeError handling by logging a warning with exception details.
src/pysonar_scanner/configuration/configuration_loader.py	Added an info log to indicate when the configuration properties are being loaded from sources.
src/pysonar_scanner/configuration/cli.py	Introduced logging imports to support logging in command line configuration handling.
src/pysonar_scanner/app_logging.py	Created a get_logger() helper function for a consistent logging interface across modules.
src/pysonar_scanner/api.py	Logged the download action for the analysis engine, aiding in tracking API interactions.
src/pysonar_scanner/main.py	Added multiple log statements to track application startup, configuration updates, and version checking.

Seppli11

Thanks for adding log statements. I think this will really help us in the long run.

Personally though, I feel like we need to log very little additional information when --verbose isn't enabled. Therefore, IMHO, most info messages in this PR should be debug messages.
On the other hand, I think we log not enough when verbose is enabled - especially dynamic properties, like where has this file been loaded from.

Sorry about the vague ticket. I should have been more precise.

Seppli11 · 2025-03-31T13:42:36Z

    logging.getLogger().setLevel(logging.DEBUG if verbose else logging.INFO)
+
+
+def get_logger() -> logging.Logger:


I'm not sure I understand why the indirection. Why not just use logging.info, logging.debug, ...

It was in the case we wanted to have a more refined logger like a named one or different loggers depending of the context. I removed it for now as we want something simple.

Seppli11 · 2025-03-31T13:44:44Z


-def check_version(api):
+def check_version(api: SonarQubeApi):
+    app_logging.get_logger().info("Checking SonarQube version...")


Not sure if this needs to be logged

Seppli11 · 2025-03-31T13:45:49Z

+        app_logging.get_logger().info("Analysis will be run on SonarQube Cloud")
        return
    version = api.get_analysis_version()
+    app_logging.get_logger().info("Analysis will be run on SonarQube version %s", version)


I feel it would be more useful to print the url, instead of these two logging statements

Seppli11 · 2025-03-31T13:46:13Z



 def create_scanner_engine(api, cache_manager, config):
+    app_logging.get_logger().info("Creating the scanner engine...")


Suggested change

app_logging.get_logger().info("Creating the scanner engine...")

Seppli11 · 2025-03-31T13:47:23Z

        Alternative, if the file IO fails, an IOError or OSError can be raised.
        """
-
+        app_logging.get_logger().info("Download the analysis engine...")


I think this logging statement should be part of the ScannerEngineProvisioner.provision method and probably should be debug level

I removed it

Seppli11 · 2025-03-31T13:49:19Z

        engine_info = self.api.get_analysis_engine()
        cache_file = self.cache.get_file(engine_info.filename, engine_info.sha256)
        if not cache_file.is_valid():
+            app_logging.get_logger().info("Cached analysis engine is not valid.")


Suggested change

app_logging.get_logger().info("Cached analysis engine is not valid.")

app_logging.get_logger().debug("No valid cached analysis engine jar was found")

Seppli11 · 2025-03-31T13:51:19Z

-        except Exception:
-            # If there's any error parsing the TOML file, return empty TomlProperties
-            # SCANPY-135: We should log the pyproject.toml parsing error
+        except Exception as e:


It would be nice to also have a debug log statement from where we loaded pyproject.toml and one if we didn't find a pyproject.toml file.
e.g.

logging.debug(f"pyproject.toml loaded from \"{filepath}\"")

and

logging.debug(f"No pyproject.toml at \"{filepath}\"")

Same applies for the sonar_project_properties module. (e.g. have an debug message which file has been loaded and when nothing has been found)

Seppli11 · 2025-03-31T13:56:43Z

+                "The JSON in SONAR_SCANNER_JSON_PARAMS environment variable is invalid. The other environment variables will still be loaded.",
+                e,
+            )
+


I could see it making sense having a debug statement which prints which environment variable have been loaded.

Suggested change

logging.debug("Loaded environment properties: " + ", ".join(f"key=value" for key,value in properties.items()))

Seppli11 · 2025-03-31T14:03:15Z

    api = build_api(config)
    check_version(api)
    update_config_with_api_urls(config, api.base_urls)
+    app_logging.get_logger().debug(f"Configuration after update from API URL: {config}")


Suggested change

app_logging.get_logger().debug(f"Configuration after update from API URL: {config}")

app_logging.get_logger().debug(f"Final loaded configuration: {config}")

Seppli11 · 2025-03-31T14:03:56Z

+    app_logging.get_logger().info("Starting Pysonar, the Sonar scanner CLI for Python")
    config = ConfigurationLoader.load()
    set_logging_options(config)
+    app_logging.get_logger().debug(f"Loaded configuration: {config}")


Suggested change

app_logging.get_logger().debug(f"Loaded configuration: {config}")

sonarqube-next · 2025-04-01T12:45:16Z

Quality Gate passed for 'Python Scanner'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

Seppli11

LGTM!

Thanks for the quick changes 💪

thomas-serre-sonarsource force-pushed the SCANPY-135 branch 3 times, most recently from 631f874 to c99c5ab Compare March 31, 2025 13:31

thomas-serre-sonarsource requested review from Seppli11 and Copilot March 31, 2025 13:33

Copilot AI reviewed Mar 31, 2025

View reviewed changes

Seppli11 requested changes Mar 31, 2025

View reviewed changes

SCANPY-135 Add log messages through the code

0afa91e

thomas-serre-sonarsource force-pushed the SCANPY-135 branch 4 times, most recently from e19deee to d489c5b Compare April 1, 2025 12:41

fix after review

e026662

thomas-serre-sonarsource force-pushed the SCANPY-135 branch from d489c5b to e026662 Compare April 1, 2025 12:44

thomas-serre-sonarsource requested a review from Seppli11 April 1, 2025 12:45

Seppli11 approved these changes Apr 1, 2025

View reviewed changes

Seppli11 merged commit 3cf8570 into master Apr 1, 2025
15 checks passed

Seppli11 deleted the SCANPY-135 branch April 1, 2025 12:56

		logging.getLogger().setLevel(logging.DEBUG if verbose else logging.INFO)


		def get_logger() -> logging.Logger:



		def create_scanner_engine(api, cache_manager, config):
		app_logging.get_logger().info("Creating the scanner engine...")

	app_logging.get_logger().info("Cached analysis engine is not valid.")
	app_logging.get_logger().debug("No valid cached analysis engine jar was found")


	logging.debug("Loaded environment properties: " + ", ".join(f"key=value" for key,value in properties.items()))

	app_logging.get_logger().debug(f"Configuration after update from API URL: {config}")
	app_logging.get_logger().debug(f"Final loaded configuration: {config}")

Conversation

thomas-serre-sonarsource commented Mar 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Seppli11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqube-next Bot commented Apr 1, 2025

Quality Gate passed for 'Python Scanner'

Uh oh!

Seppli11 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Seppli11 left a comment •

edited

Loading