Skip to content

add routes logging message #37

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add routes logging message #37

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2dd13d6
add routes logging message
Nuovaxu Aug 10, 2024
7a5bda9
remove unncessary log messages from exception
Nuovaxu Sep 22, 2024
4e527fe
chore: switch to % formatting in logs for deferred computation
Jan 5, 2025
fab002c
Removed logs preceding abort() calls as they are now handled by the g…
Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions application/routes/audio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,27 @@
from .. import api
from ..codes import ConnectionType
from ..features.Audio import AUDIO_PORT

import logging
logger = logging.getLogger(__name__)

@api.route('/audio', methods=['POST'])
def start_audio():
#TODO: Request recieved with body
logger.debug("Audio: Received request to start audio with data: {}".format(request.json))
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improve logging and remove TODO comment.

The debug log is a good addition for tracing requests. However, there are a couple of improvements we can make:

  1. Remove the TODO comment as it's no longer needed.
  2. Use an f-string for better readability and slight performance improvement.

Here's the suggested change:

- #TODO: Request recieved with body
- logger.debug("Audio: Received request to start audio with data: {}".format(request.json))
+ logger.debug(f"Audio: Received request to start audio with data: {request.json}")

This change addresses the static analysis hints (UP032 and G001) while maintaining the logging functionality.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#TODO: Request recieved with body
logger.debug("Audio: Received request to start audio with data: {}".format(request.json))
logger.debug(f"Audio: Received request to start audio with data: {request.json}")
🧰 Tools
🪛 Ruff

44-44: Use f-string instead of format call

Convert to f-string

(UP032)

44-44: Logging statement uses str.format

(G001)

session_id = request.json.get('session_id')
audio, reason = create_connection(session_id, ConnectionType.AUDIO)
if reason != '':
logger.warning("Audio: Failed to create audio connection: {}".format(reason))
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improve logging statements with f-strings.

The addition of warning and error logs for failure scenarios is excellent. To further improve the code, let's use f-strings as suggested by the static analysis:

Apply the following changes:

- logger.warning("Audio: Failed to create audio connection: {}".format(reason))
+ logger.warning(f"Audio: Failed to create audio connection: {reason}")

- logger.error("Audio: Audio launch failed: {}".format(reason))
+ logger.error(f"Audio: Audio launch failed: {reason}")

These changes address the static analysis hints (UP032 and G001) while maintaining the logging functionality.

Also applies to: 53-53

🧰 Tools
🪛 Ruff

48-48: Use f-string instead of format call

Convert to f-string

(UP032)

48-48: Logging statement uses str.format

(G001)

abort(403, description=reason)

status, reason = audio.launch_audio()
if status is False:
logger.error("Audio: Audio launch failed: {}".format(reason))
abort(403, description=reason)

result = {
'port': AUDIO_PORT,
'audio_id': audio.id
}

logger.info("Audio: Audio launched successfully with ID {} on port {}".format(audio.id, AUDIO_PORT))
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improve dictionary formatting and logging statement.

The addition of an info log for successful audio launch is great. Let's make a few minor improvements:

  1. Add a trailing comma to the last item in the result dictionary for better maintainability.
  2. Use an f-string for the logging statement.

Apply the following changes:

 result = {
     'port': AUDIO_PORT,
-    'audio_id': audio.id
+    'audio_id': audio.id,
 }
-logger.info("Audio: Audio launched successfully with ID {} on port {}".format(audio.id, AUDIO_PORT))
+logger.info(f"Audio: Audio launched successfully with ID {audio.id} on port {AUDIO_PORT}")

These changes address the static analysis hints (COM812, UP032, and G001) while maintaining the functionality.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
'audio_id': audio.id
}
logger.info("Audio: Audio launched successfully with ID {} on port {}".format(audio.id, AUDIO_PORT))
result = {
'port': AUDIO_PORT,
'audio_id': audio.id,
}
logger.info(f"Audio: Audio launched successfully with ID {audio.id} on port {AUDIO_PORT}")
🧰 Tools
🪛 Ruff

58-58: Trailing comma missing

Add trailing comma

(COM812)

60-60: Use f-string instead of format call

Convert to f-string

(UP032)

60-60: Logging statement uses str.format

(G001)

return json.dumps(result)
20 changes: 16 additions & 4 deletions application/routes/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@
from ..features.Term import Term
from ..features.VNC import VNC
from ..utils import int_to_bytes

import logging
logger = logging.getLogger(__name__)

def create_connection(session_id, conn_type):
logger.debug(f"Common: Attempting to create connection: session_id={session_id}, conn_type={conn_type}")
host, username, this_private_key_path, this_private_key_str, _ = profiles.get_session_info(session_id)
if host is None:
logger.warning(f"Common: Session not found: {session_id}")
abort(403, f'Fail: session {session_id} does not exist')

if conn_type == ConnectionType.GENERAL:
Expand All @@ -49,11 +52,13 @@ def create_connection(session_id, conn_type):
elif conn_type == ConnectionType.AUDIO:
conn = Audio()
else:
logger.error(f"Common: Invalid connection type requested: {conn_type}")
raise TypeError(f'Invalid type: {conn_type}')

status, reason = conn.connect(host, username,
key_filename=this_private_key_path, private_key_str=this_private_key_str)
if status is False:
logger.error(f"Common: Connection failed: {reason}")
if reason.startswith('[Errno 60]') \
or reason.startswith('[Errno 64]') \
or reason.startswith('[Errno 51]') \
Expand All @@ -71,11 +76,13 @@ def create_connection(session_id, conn_type):

@api.route('/profiles')
def get_profiles():
logger.info("Common: Fetching all profiles")
return profiles.query()


@api.route('/session', methods=['GET', 'POST', 'PATCH', 'DELETE'])
def handle_session():
logger.debug(f"Common: Session operation: {request.method}")
if request.method == 'GET':
session_id = request.args.get('id')
host, username, _, _, nickname = profiles.get_session_info(session_id)
Expand All @@ -90,16 +97,18 @@ def handle_session():
username = request.json.get('username')
# FIXME: password should be optional
password = request.json.get("password")

logger.debug("Common: Creating new session")
conn = Connection()

status, reason = conn.connect(host, username, password=password)
if status is False:
logger.warning(f"Common: Failed to create session: {reason}")
abort(403, reason)

# FIXME: password should be optional: only pass 'conn' if password is given
status, reason = profiles.add_session(host, username, conn)
if status is False:
logger.error("Common: Failed to save session info")
abort(500, reason)

return 'success'
Expand All @@ -112,10 +121,12 @@ def handle_session():

if nickname is not None:
# only update nickname
logger.info(f"Common: Updating nickname for session {session_id}")
status, reason = profiles.set_session_nickname(session_id, nickname)
if not status:
abort(400, reason)
else:
logger.info("Common: Terminating old session")
# terminate old sessions with best efforts
# noinspection PyBroadException
try:
Expand Down Expand Up @@ -145,7 +156,7 @@ def handle_session():

elif request.method == 'DELETE':
session_id = request.args.get('session_id')

logger.info(f"Common: Deleting session {session_id}")
status, reason = profiles.delete_session(session_id)
if not status:
abort(403, reason)
Expand All @@ -160,7 +171,7 @@ def exec_blocking():
session_id = request.json.get('session_id')
cmd = request.json.get('cmd')
large = request.json.get('large', False)

logger.debug("Common: Executing blocking command")
conn, reason = create_connection(session_id, ConnectionType.GENERAL)
if reason != '':
abort(403, reason)
Expand All @@ -171,6 +182,7 @@ def exec_blocking():
else:
status, _, stdout, stderr = conn.exec_command_blocking(cmd)
if status is False:
logger.error("Common: Command execution failed")
abort(500, 'exec failed')
result = '\n'.join(stdout) + '\n' + '\n'.join(stderr)

Expand Down
37 changes: 29 additions & 8 deletions application/routes/sftp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
import posixpath
from datetime import datetime
from pathlib import Path

import logging
logger = logging.getLogger(__name__)
from flask import request, abort, stream_with_context

from .common import create_connection
Expand All @@ -36,28 +37,33 @@
def sftp_ls(session_id):
path = request.args.get('path')

logger.debug(f"Sftp: Listing SFTP directory: {path} for session {session_id}")
sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

status, cwd, file_list = sftp.ls(path)
if status is False:
logger.error(f"Sftp: Failed to list directory: {cwd}")
abort(400, description=cwd)

result = {
'cwd': cwd,
'files': file_list
}
logger.info(f"Sftp: Directory listed successfully: {cwd}")
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Logging added to sftp_ls function.

The added logging statements provide good coverage of the function's execution flow, including the start of the operation, connection failure, directory listing failure, and successful completion. The log levels (debug, error, info) are appropriately used.

Minor suggestion: Consider using logger.exception() instead of logger.error() when logging errors, as it automatically includes the stack trace, which can be helpful for debugging.

🧰 Tools
🪛 Ruff

40-40: Logging statement uses f-string

(G004)

43-43: Logging statement uses f-string

(G004)

48-48: Logging statement uses f-string

(G004)

53-53: Trailing comma missing

Add trailing comma

(COM812)

55-55: Logging statement uses f-string

(G004)

return json.dumps(result)


@api.route('/sftp_dl/<session_id>')
def sftp_dl(session_id):
cwd = request.args.get('cwd')
args_files = request.args.get('files')

logger.debug(f"Sftp: Downloading files: {args_files} from {cwd} for session {session_id}")
sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

files = json.loads(args_files)
Expand All @@ -75,11 +81,12 @@ def sftp_dl(session_id):
dt_str = datetime.now().strftime('_%Y%m%d_%H%M%S')
zip_name = posixpath.basename(cwd) + dt_str + '.zip'
r.headers.set('Content-Disposition', 'attachment', filename=zip_name)
logger.info(f"Sftp: Sending zip file: {zip_name}")
else:
r = app.response_class(stream_with_context(sftp.dl_generator(files[0])), mimetype='application/octet-stream')
r.headers.set('Content-Disposition', 'attachment', filename=files[0])
r.headers.set('Content-Length', size)

logger.info(f"Sftp: Sending file: {files[0]}")
return r


Expand All @@ -88,15 +95,17 @@ def sftp_rename(session_id):
cwd = request.json.get('cwd')
old = request.json.get('old')
new = request.json.get('new')

logger.debug(f"Sftp: Renaming file from {old} to {new} in {cwd} for session {session_id}")
sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

status, reason = sftp.rename(cwd, old, new)
if not status:
logger.error(f"Sftp: Rename failed: {reason}")
abort(400, reason)

logger.info("Sftp: Rename successful")
return 'success'


Expand All @@ -105,31 +114,38 @@ def sftp_chmod(session_id):
path = request.json.get('path')
mode = request.json.get('mode')
recursive = request.json.get('recursive')
logger.debug(f"Sftp: Changing file mode for {path} to {mode}, recursive: {recursive} in session {session_id}")

sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

status, reason = sftp.chmod(path, mode, recursive)
if not status:
logger.error(f"Sftp: CHMOD failed: {reason}")
abort(400, reason)

logger.info("Sftp: CHMOD successful")
return 'success'


@api.route('/sftp_mkdir/<session_id>', methods=['PUT'])
def sftp_mkdir(session_id):
cwd = request.json.get('cwd')
name = request.json.get('name')

logger.debug(f"Sftp: Creating directory {name} in {cwd} for session {session_id}")
sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

status, reason = sftp.mkdir(cwd, name)
if status is False:
logger.error(f"Sftp: Directory creation failed: {reason}")
abort(400, description=reason)

logger.info("Sftp: Directory created successfully")
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Logging added to sftp_mkdir function. Consider adding type annotations.

The added logging statements provide comprehensive coverage of the function's execution flow, including the start of the mkdir operation, connection failure, directory creation failure, and successful completion. The log levels (debug, error, info) are appropriately used.

Suggestion for improvement:
Add type annotations to the function signature:

def sftp_mkdir(session_id: str) -> str:
🧰 Tools
🪛 Ruff

134-134: Missing return type annotation for public function sftp_mkdir

Add return type annotation: str

(ANN201)

134-134: Missing type annotation for function argument session_id

(ANN001)

137-137: Logging statement uses f-string

(G004)

140-140: Logging statement uses f-string

(G004)

145-145: Logging statement uses f-string

(G004)

return 'success'


Expand All @@ -139,9 +155,11 @@ def sftp_ul(session_id):
# no need to use secure_filename because the user should be responsible for her/his input
# when not using the client
relative_path = request.headers.get('Path')
logger.debug(f"Sftp: Uploading file {relative_path} to {cwd} for session {session_id}")

sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

p = Path(relative_path)
Expand All @@ -162,21 +180,24 @@ def sftp_ul(session_id):
chunk = request.stream.read(UPLOAD_CHUNK_SIZE)

sftp_file.close()

logger.info(f"Sftp: File uploaded successfully: {request_filename}")
return 'success'


@api.route('/sftp_rm/<session_id>', methods=['POST'])
def sftp_rm(session_id):
cwd = request.json.get('cwd')
files = request.json.get('files')

logger.debug(f"Sftp: Removing files {files} from {cwd} for session {session_id}")
sftp, reason = create_connection(session_id, ConnectionType.SFTP)
if reason != '':
logger.error(f"Sftp: SFTP connection failed: {reason}")
abort(403, description=reason)

status, reason = sftp.rm(cwd, files)
if not status:
logger.error(f"Sftp: File removal failed: {reason}")
abort(400, description=reason)

logger.info("Sftp: Files removed successfully")
return 'success'
13 changes: 11 additions & 2 deletions application/routes/term.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
from ..codes import ICtrlStep, ConnectionType, ICtrlError
from ..features.Term import TERM_CONNECTIONS, TERMINAL_PORT
from ..utils import int_to_bytes
import logging
logger = logging.getLogger(__name__)


# FIXME: store term_id is cookie-based Flask 'session'
Expand All @@ -35,30 +37,34 @@
def start_terminal():
session_id = request.json.get('sessionID')
load_check = request.json.get('loadCheck', True)

logger.debug(f"Term: Starting terminal session: {session_id}")
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Informative debug log

The debug log message is informative and includes the session ID, which is helpful for tracing.

Note: The static analysis tool flagged the use of an f-string (G004). However, f-strings are generally considered more readable and performant in Python 3.6+. Unless there's a specific project guideline against f-strings, this usage is appropriate.

🧰 Tools
🪛 Ruff

40-40: Logging statement uses f-string

(G004)

def generate():
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding a return type annotation to generate().
Since this function yields data to the client, specifying a -> Generator[bytes, None, None] or similar can improve readability and tooling support.

 def generate():
+    # type: (...) -> Generator[bytes, None, None]
     ...

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.8.2)

41-41: Missing return type annotation for private function generate

(ANN202)

yield int_to_bytes(ICtrlStep.Term.SSH_AUTH)

term, reason = create_connection(session_id, ConnectionType.TERM)
if reason != '':
logger.error(f"Term: Terminal connection failed: {reason}")
yield reason
return

yield int_to_bytes(ICtrlStep.Term.CHECK_LOAD)
if term.is_uoft() and load_check and term.is_load_high():
logger.warning(f"Term: Load too high to start terminal session: {session_id}")
yield int_to_bytes(ICtrlError.SSH.OVER_LOADED)
return

yield int_to_bytes(ICtrlStep.Term.LAUNCH_SHELL)
status, reason = term.launch_shell()
if status is False:
logger.error(f"Term: Failed to launch terminal shell: {reason}")
abort(403, description=reason)

yield int_to_bytes(ICtrlStep.Term.DONE)
result = {
'port': TERMINAL_PORT,
'term_id': term.id
}
logger.info(f"Term: Terminal session started successfully: {term.id} on port {TERMINAL_PORT}")
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Informative log for successful session start with minor style suggestion

The info log is well-placed and includes important details like the terminal ID and port number. This will be helpful for monitoring successful session starts.

Consider adding a trailing comma after the 'term_id' key-value pair in the result dictionary for consistency with Python style guides:

 result = {
     'port': TERMINAL_PORT,
-    'term_id': term.id
+    'term_id': term.id,
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
'term_id': term.id
}
logger.info(f"Term: Terminal session started successfully: {term.id} on port {TERMINAL_PORT}")
'port': TERMINAL_PORT,
'term_id': term.id,
}
logger.info(f"Term: Terminal session started successfully: {term.id} on port {TERMINAL_PORT}")
🧰 Tools
🪛 Ruff

65-65: Trailing comma missing

Add trailing comma

(COM812)

67-67: Logging statement uses f-string

(G004)

yield json.dumps(result)

return app.response_class(stream_with_context(generate()), mimetype='application/octet-stream')
Expand All @@ -68,14 +74,17 @@ def generate():
def resize_terminal():
term_id = request.json.get('term_id')
if term_id not in TERM_CONNECTIONS:
logger.error(f"Term: Invalid terminal ID for resize: {term_id}")
abort(403, description='invalid term_id')

width = request.json.get('w')
height = request.json.get('h')

logger.debug(f"Term: Resizing terminal {term_id}: width {width}, height {height}")
term = TERM_CONNECTIONS[term_id]
status, reason = term.resize(width, height)
if status is False:
logger.error(f"Term: Failed to resize terminal {term_id}: {reason}")
abort(403, description=reason)

logger.info(f"Term: Terminal {term_id} resized successfully")
return 'success'
Loading
Loading