Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added log config and replaced print with log #235

Closed
wants to merge 6 commits into from

Conversation

rjarun8
Copy link

@rjarun8 rjarun8 commented Jul 1, 2023

Description

I have replaced the print() function with the logging.info() function in the scanner.py file. This change allows the program to output log messages with timestamps, which is useful for tracking the progress of long-running scans.

Changes Made

  • Replaced print() function with logging.info() in scanner.py.
  • Configured the logging module to output log messages with timestamps.

Checklist

  • I have read and followed the contributing guidelines.
  • I have tested my changes thoroughly and they work as expected.
  • I have added necessary tests for the changes made.
  • I have updated the documentation to reflect the changes made.
  • My code follows the project's coding style and standards.
  • I have added appropriate commit messages and comments for my changes.

Related Issues

  • Issue #143: Use standardised log output with timestamp for all output

Additional Notes

This change should make it easier to track the progress of long-running scans. The log messages now include a timestamp, which can be helpful for understanding when each event occurred. I have not added any new tests or updated the documentation, as I believe these changes are self-explanatory and do not significantly alter the functionality of the program.

@google-cla
Copy link

google-cla bot commented Jul 1, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mshudrak
Copy link
Collaborator

mshudrak commented Jul 1, 2023

Please accept Google's CLA in order to proceed.

Copy link
Collaborator

@mshudrak mshudrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL at my comments.

@@ -166,7 +169,7 @@ def crawl_loop(initial_sa_tuples: List[Tuple[str, Credentials, List[str]]],

project_id = project['projectId']
project_number = project['projectNumber']
print(f'Inspecting project {project_id}')
logging.info(f'Inspecting project {project_id}')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually want this to be printed in console so user understand what's going on.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using logging for everything makes sense here and have the default log level be info. The output is metadata about the process, not the result of the process itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using logging for everything makes sense here and have the default log level be info. The output is metadata about the process, not the result of the process itself.

Will there always be a logging file as part of the arguments? coz I was thinking if that file name is optional and not mandatory we can separate the logging as a filehandle for file logging and console handler for console printing

something like this

import os
import logging

def main():
    log_level = getattr(logging, 'INFO', None)
    log_format = '%(asctime)s - %(levelname)s - %(message)s'
    date_format = '%Y-%m-%d %H:%M:%S'
    
    # Create a formatter
    formatter = logging.Formatter(fmt=log_format, datefmt=date_format)

    # Get the root logger
    logger = logging.getLogger()
    logger.setLevel(log_level)

    # Check if the log file exists
    if os.path.exists('my_log_file.log'):
        # If the log file exists, create a handler for the log file
        file_handler = logging.FileHandler(filename='my_log_file.log', mode='a')
        file_handler.setFormatter(formatter)
        logger.addHandler(file_handler)
    else:
        # If the log file does not exist, create a handler for the console
        console_handler = logging.StreamHandler()
        console_handler.setFormatter(formatter)
        logger.addHandler(console_handler)

    logger.info("this is a new line")

main()  # Call the main function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using logging for everything makes sense here and have the default log level be info. The output is metadata about the process, not the result of the process itself.

Yea, probably that's how it should be. I was thinking about having INFO to be default but in some cases it is just a lot of data in terminal. IMO, it would be great to allow user to specify verbosity level instead of just ERROR, WARNING, INFO, with default (verbosity 0 or 1) to be minimum required to understand that scanner is working and scanning projects (probably print now) and maximum (e.g. 3) where we print everything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using logging for everything makes sense here and have the default log level be info. The output is metadata about the process, not the result of the process itself.

Will there always be a logging file as part of the arguments? coz I was thinking if that file name is optional and not mandatory we can separate the logging as a filehandle for file logging and console handler for console printing

something like this

import os
import logging

def main():
    log_level = getattr(logging, 'INFO', None)
    log_format = '%(asctime)s - %(levelname)s - %(message)s'
    date_format = '%Y-%m-%d %H:%M:%S'
    
    # Create a formatter
    formatter = logging.Formatter(fmt=log_format, datefmt=date_format)

    # Get the root logger
    logger = logging.getLogger()
    logger.setLevel(log_level)

    # Check if the log file exists
    if os.path.exists('my_log_file.log'):
        # If the log file exists, create a handler for the log file
        file_handler = logging.FileHandler(filename='my_log_file.log', mode='a')
        file_handler.setFormatter(formatter)
        logger.addHandler(file_handler)
    else:
        # If the log file does not exist, create a handler for the console
        console_handler = logging.StreamHandler()
        console_handler.setFormatter(formatter)
        logger.addHandler(console_handler)

    logger.info("this is a new line")

main()  # Call the main function

The log filename argument is optional. If it is set, we print in the file and if not the argument is None and it is simply console printing as far as I remember.

@@ -26,6 +26,9 @@
from pathlib import Path
from typing import List, Tuple, Dict, Optional, Union

# Configure the logging module
logging.basicConfig(format='%(asctime)s %(message)s', level=logging.INFO, datefmt='%Y-%m-%d %H:%M:%S')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have it already here:

logging.basicConfig(level=getattr(logging, args.log_level.upper(), None),

@rjarun8
Copy link
Author

rjarun8 commented Jul 1, 2023

Please accept Google's CLA in order to proceed.

Accepted

src/gcp_scanner/scanner.py Outdated Show resolved Hide resolved
@@ -197,7 +197,9 @@ def crawl_loop(initial_sa_tuples: List[Tuple[str, Credentials, List[str]]],
continue

project_id = project['projectId']
print(f'Inspecting project {project_id}')
# print(f'Inspecting project {project_id}')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't leave old code commented out. It's in the history anyway.

@mshudrak
Copy link
Collaborator

Just noticed activity in this PR, sorry for not replying later. Like I mentioned in the comment, I left it to be print and not logging.info on purpose. Otherwise GCP Scanner is completely silent and user has 0 idea of what's going on and whether it is working at all. We either need to have another level of verbosity that's printing some basic level of information in terminal or just leave as it is now with print.

@ZetaTwo
Copy link
Collaborator

ZetaTwo commented Jul 17, 2023

Why can't we set the default level to info and use logging?

@mxmssh
Copy link
Collaborator

mxmssh commented Jul 17, 2023 via email

@ZetaTwo
Copy link
Collaborator

ZetaTwo commented Jul 17, 2023

Doesn't that suggest that we should shift a lot of the info messages to debug messages?

@mxmssh
Copy link
Collaborator

mxmssh commented Jul 17, 2023 via email

@mshudrak
Copy link
Collaborator

mshudrak commented Aug 8, 2023

I am closing this one due to inactivity. TL;DR: we need a logging system with multiple levels of verbosity.

@mshudrak mshudrak closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants