-
-
Notifications
You must be signed in to change notification settings - Fork 274
debugging functionality added #1569
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
.PHONY: install-devel check lint test check-clean-tree fix force-fix venv | ||
.PHONY: install-devel check lint test check-clean-tree fix force-fix venv debug debug-profile debug-clean | ||
|
||
# NOTE: ZT_VENV and BASEPYTHON are advanced undocumented features | ||
# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>" | ||
|
@@ -24,6 +24,20 @@ lint: venv | |
test: venv | ||
@pytest | ||
|
||
### DEBUG TARGETS ### | ||
|
||
debug: venv | ||
@echo "=== Running with debug enabled ===" | ||
$(PYTHON) -m zulipterminal.cli.run -d | ||
|
||
debug-profile: venv | ||
@echo "=== Running with profiling enabled ===" | ||
$(PYTHON) -m zulipterminal.cli.run --profile | ||
|
||
Comment on lines
+29
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern with these is that the extra options available to the While eg. themes could be debugged by changing the config file, some options are only available on the command-line, such as 'explore mode' right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about making
|
||
debug-clean: | ||
@echo "=== Cleaning debug files ===" | ||
rm -f debug.log zulip-terminal.prof zulip-terminal-tracebacks.log | ||
|
||
Comment on lines
+37
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This certainly cleans files from the current folder, but not necessarily from where the files are accumulating. Here I've only seen profile files in I'm also a little wary of making it too easy to delete the other files, since while that information can accumulate and it's not clear which is worth keeping, it can also often be really useful! I suspect a more refined solution could be to build upon the move to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you're totally right. I had a messy gh codespace that I was organizing everything at that moment, so I may had a mistake about temp files. Cleaning files, especially the logs right now make me concern about changes more right now too. Yes, logs are long and hard to read files but deleting could make feel losing daily to-do. What about action of keeping an option for normal - developer mode so normal users just delete logs & devs keep them |
||
### FIX FILES ### | ||
|
||
check-clean-tree: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,7 @@ def long_description(): | |
helper_deps = [ | ||
"pudb==2022.1.1", | ||
"snakeviz>=2.1.1", | ||
"requests>=2.25.0", # Added for debug_helper.py | ||
] | ||
|
||
setup( | ||
|
@@ -93,6 +94,8 @@ def long_description(): | |
"console_scripts": [ | ||
"zulip-term = zulipterminal.cli.run:main", | ||
"zulip-term-check-symbols = zulipterminal.scripts.render_symbols:main", | ||
# Added debug helper with proper path | ||
"zulip-term-debug = zulipterminal.scripts.debug_helper:main", | ||
Comment on lines
+97
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes it available to users who install the basic package, not just run it from the source tree; is that the intention? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was the idea. Making the debug_helper available to users who install the basic package means they can use the debugging tools whether they’re running from the source tree or from an installed environment. If this brings up any concerns, I’m down to discuss it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be helpful because more input about problems helps us solve them more effectively |
||
], | ||
}, | ||
extras_require={ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,164 @@ | ||
#!/usr/bin/env python3 | ||
""" | ||
Helper script for debugging Zulip Terminal. | ||
|
||
This script provides utilities for common debugging tasks: | ||
1. Analyzing debug logs | ||
2. Testing connectivity to Zulip server | ||
3. Checking terminal capabilities | ||
""" | ||
|
||
import argparse | ||
import json | ||
import logging | ||
import os | ||
import re | ||
import subprocess | ||
from typing import Optional | ||
|
||
|
||
# Configure logging | ||
logging.basicConfig(level=logging.INFO) | ||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def analyze_debug_log(log_file: str = "debug.log") -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this tool were to exist, it would be useful to identify multiple possible files. We have a tracebacks file, an API log, and a thread-exceptions log as it stands. I wouldn't expect debug.log to necessarily have the error_patterns you mention, since anything can be 'print'ed into them, so having a default of debug.log with those patterns is surprising. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. if the tool's for analyzing logs properly, it should handle multiple log files like tracebacks, API logs, and thread-exceptions. Limiting it to A better approach: |
||
""" | ||
Analyze a debug log file for common issues. | ||
""" | ||
if not os.path.exists(log_file): | ||
logger.error("Log file '%s' not found", log_file) | ||
return | ||
|
||
logger.info("Analyzing %s...", log_file) | ||
with open(log_file, "r") as f: | ||
content = f.read() | ||
|
||
# Look for error patterns | ||
error_patterns = [r"ERROR", r"Exception", r"Traceback", r"Failed to"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can find lots of issues (my files are quite large right now!), but could you give an example of how it has helped you? Do you have a specific motivation for this? I would expect that reading through the file to get multiple lines of context would be more helpful? If this is also for reporting errors to maintainers/developers, then context can be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I added this is to quickly spot common error patterns, which helps a lot with large files. But yeah, I get it — without context, it’s not super helpful. Like, finding an ERROR by itself doesn’t really tell us why it happened or what led up to it. |
||
|
||
errors_found = False | ||
for pattern in error_patterns: | ||
matches = re.finditer(pattern, content, re.IGNORECASE) | ||
for match in matches: | ||
line_start = content.rfind("\n", 0, match.start()) + 1 | ||
line_end = content.find("\n", match.end()) | ||
if line_end == -1: | ||
line_end = len(content) | ||
|
||
line = content[line_start:line_end].strip() | ||
logger.warning("Potential issue found: %s", line) | ||
errors_found = True | ||
|
||
if not errors_found: | ||
logger.info("No obvious errors found in the log file.") | ||
|
||
|
||
def test_connectivity(server_url: Optional[str] = None) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the main zulip-term script does check for connectivity, and the server version can be listed from within it, I can see a simple tool along these lines being useful. However, rather than having a script with semi-duplicate code from There is plenty of server information available other than the version, which could also be used to help with authentication issues, for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think good approach would be to refactor the relevant logic in |
||
""" | ||
Test connectivity to a Zulip server. | ||
""" | ||
if not server_url: | ||
# Try to get server URL from zuliprc | ||
zuliprc_path = os.path.expanduser("~/.zuliprc") | ||
if os.path.exists(zuliprc_path): | ||
with open(zuliprc_path, "r") as f: | ||
for line in f: | ||
if line.startswith("site="): | ||
server_url = line.split("=")[1].strip() | ||
break | ||
|
||
if not server_url: | ||
logger.error("No server URL provided and couldn't find one in ~/.zuliprc") | ||
return | ||
|
||
logger.info("Testing connectivity to %s...", server_url) | ||
try: | ||
import requests | ||
|
||
response = requests.get(f"{server_url}/api/v1/server_settings") | ||
if response.status_code == 200: | ||
logger.info("Successfully connected to %s", server_url) | ||
try: | ||
settings = response.json() | ||
logger.info( | ||
"Server version: %s", settings.get("zulip_version", "unknown") | ||
) | ||
except json.JSONDecodeError: | ||
logger.error("Received response, but couldn't parse as JSON") | ||
else: | ||
logger.error("Failed to connect: HTTP status %s", response.status_code) | ||
except Exception as e: | ||
logger.error("Connection error: %s", e) | ||
|
||
|
||
def check_terminal_capabilities() -> None: | ||
""" | ||
Check for terminal capabilities that might affect Zulip Terminal. | ||
""" | ||
logger.info("Checking terminal capabilities...") | ||
|
||
# Check for color support | ||
colors = os.environ.get("TERM", "unknown") | ||
logger.info("TERM environment: %s", colors) | ||
|
||
if "COLORTERM" in os.environ: | ||
logger.info("COLORTERM: %s", os.environ["COLORTERM"]) | ||
|
||
# Check for Unicode support | ||
logger.info("Testing Unicode rendering capabilities:") | ||
test_chars = [ | ||
("Basic symbols", "▶ ◀ ✓ ✗"), | ||
("Emoji (simple)", "😀 🙂 👍"), | ||
("Box drawing", "│ ┌ ┐ └ ┘ ├ ┤ ┬ ┴ ┼"), | ||
("Math symbols", "∞ ∑ √ ∫ π"), | ||
] | ||
Comment on lines
+101
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like something that could belong in That existing tool checks the symbols that are currently being used, rather than an assortment of sample ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, this is similar to what render_symbols.py does, but there's a difference. This script tests a range of symbols for general Unicode rendering in the terminal, while render_symbols.py focuses on the symbols used specifically in Zulip Terminal. If the goal is to extend render_symbols.py for broader Unicode testing, this code could be added as a feature. Otherwise, it could stay separate as part of a general terminal debugging tool. What do you think—integrate it or keep it separate? |
||
|
||
for name, chars in test_chars: | ||
logger.info(" %s: %s", name, chars) | ||
|
||
|
||
def main() -> None: | ||
""" | ||
Main entry point for the debugging helper. | ||
""" | ||
parser = argparse.ArgumentParser(description="Zulip Terminal Debugging Helper") | ||
subparsers = parser.add_subparsers(dest="command", help="Command to run") | ||
|
||
# Log analyzer | ||
log_parser = subparsers.add_parser("log", help="Analyze debug logs") | ||
log_parser.add_argument("--file", default="debug.log", help="Log file to analyze") | ||
|
||
# Connectivity test | ||
conn_parser = subparsers.add_parser("connect", help="Test connectivity") | ||
conn_parser.add_argument( | ||
"--server", help="Server URL (e.g., https://chat.zulip.org)" | ||
) | ||
|
||
# Terminal test | ||
subparsers.add_parser("terminal", help="Check terminal capabilities") | ||
|
||
# Run zulip-term with debug | ||
run_parser = subparsers.add_parser("run", help="Run zulip-term with debugging") | ||
run_parser.add_argument("--profile", action="store_true", help="Enable profiling") | ||
|
||
args = parser.parse_args() | ||
|
||
if args.command == "log": | ||
analyze_debug_log(args.file) | ||
elif args.command == "connect": | ||
test_connectivity(args.server) | ||
elif args.command == "terminal": | ||
check_terminal_capabilities() | ||
elif args.command == "run": | ||
cmd = ["zulip-term", "-d"] | ||
if args.profile: | ||
cmd.append("--profile") | ||
logger.info("Running: %s", " ".join(cmd)) | ||
subprocess.run(cmd, check=False) | ||
Comment on lines
+153
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to simply wrap commands, as per:
What is the benefit of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since 2 weeks passed, I think I was trying to clean things up or reframe it, but I didn't add any new functionality. I can roll that back and just stick to the original commands if you want |
||
else: | ||
parser.print_help() | ||
|
||
|
||
if __name__ == "__main__": | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this compare to the section above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created because remote debugging example just takes the pudb tip and puts it into action with a real use case, like debugging send_message. It's not just explaining the tool, but showing exactly how to use it in a scenario that devs can actually relate to. That makes it way easier to follow for anyone who's never set this up before, and helps connect the dots between the theory and how it works in practice too