Fixed major bugs that affect Logicytics (performance checker) #224
Conversation
…oL changes
Logicytics.py
Fixed bug [ValueError String Formatting], where the memory was not a proper string if an application runs into an error
Thanks to new changes, `Get.list_of_files` now ignores the `logicytics` folder and `, SysInternal_Suite` folder, which resulted in bugs when executed and crashes
Added debug log to get length of execution list
Hard limited memory changes (in performance checker) to be at minimum 0 (Logging will still show you the warning), this is to exclude the edge cases where freed memory is more than used memory
Improved logging of `performance check` where messages are more in detail and memory is now in 3dp rather than 2dp
Added try-except statements to prevent crashes if code fails (performance checker)
Made function `zip_generated_files()` into a class `ZIP`, with the main and helper function in it
Get.py
Fixed bug in execution list where it included the `logicytics` library, which led to them being run, thus errors, adding new param: `exclude_dirs`
Flag.py
Fixed bug where CONFIG wasn't importing due to improper import statement (`from .Config import CONFIG` is now `from logicytics.Config import CONFIG`)
network_psutil.py
Fixed bug where __measure_network_bandwidth_usage wasn't run due to faulty async-await code, now fixed by making `get` and async function, and awaits `__measure_network_bandwidth_usage`
Improved error logging to prevent crashes.
Also, gitignore now ignore the exe files that are zipped when they are unzipped (`SysInternal suite`), Thus developers don't accidentally commit them
Signed-off-by: Shahm Najeeb <[email protected]>
Protection of `raw` logging to be `_raw` Resource heavy `inspect.stack()[1]` is now a less intensive `inspect.currentframe().f_back` Improved `message is not None` check to be more efficient Added regex to remove ANSI escape codes from being logged into the file (as they produce cluttered logs) Signed-off-by: Shahm Najeeb <[email protected]>
Added configuration settings to be changed to network_psutil.py, packet_sniffer.py and dump_memory.py Renamed global CONFIG var to config Signed-off-by: Shahm Najeeb <[email protected]>
WalkthroughThis update adds new config sections and settings, improves error handling, and refactors several modules for consistency. Key updates include renaming configuration variables from Changes
Sequence Diagram(s)%% Sequence diagram for asynchronous network measurement
sequenceDiagram
participant User
participant NetworkInfo
participant AsyncProcessor
User->>NetworkInfo: Trigger async get()
NetworkInfo->>NetworkInfo: Initialize SAMPLE_COUNT & INTERVAL from config
NetworkInfo->>NetworkInfo: Call __measure_network_bandwidth_usage(sample_count, interval)
NetworkInfo->>AsyncProcessor: Await bandwidth measurement
AsyncProcessor-->>NetworkInfo: Return measured data
NetworkInfo-->>User: Respond with network info
%% Sequence diagram for ZIP file handling in Logicytics
sequenceDiagram
participant Module as Logicytics Module
participant ZIP as ZIP Class
participant FS as FileSystem
Module->>ZIP: Call ZIP.files() for zipping files
ZIP->>FS: Retrieve necessary files (applying filters)
FS-->>ZIP: Return file data
ZIP-->>Module: Return zipped output
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
DefinetlyNotAI
added
bug/High
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
Signed-off-by: Shahm Najeeb <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
CODE/logicytics/Logger.py (1)
206-206: Remove unnecessary f-stringThere's an f-string that doesn't have any placeholders, so it can just be a regular string.
- f"Raw message called from a non-function - This is not recommended" + "Raw message called from a non-function - This is not recommended"🧰 Tools
🪛 Ruff (0.8.2)
206-206: f-string without any placeholders
Remove extraneous
fprefix(F541)
CODE/config.ini (1)
95-97: Adding 'max_retry_time' is handy.
This new setting gives users better control. Maybe add a small note in the config doc about potential wait times if they set it super high.CODE/network_psutil.py (1)
140-142: Solid critical check on sample_count and interval.
It’s super helpful to fail fast if the config is messed up. Optionally, you could handle it earlier or provide defaults to avoid a crash.CODE/packet_sniffer.py (1)
361-361: Critical logging for bad config is clutch.
Great call to stop if the config is borked. If you want to be fancy, you could offer a fallback or prompt the user to fix the config.Also applies to: 363-370
CODE/Logicytics.py (1)
494-494: Good planning with the TODO comment!Adding a TODO comment for future webhook implementation in v3.5 helps track upcoming features. Consider adding more details about what needs to be implemented.
- # TODO: Implement this in future v3.5 + # TODO: Implement webhook functionality in v3.5 to send notifications after completion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks).idea/csv-editor.xml(1 hunks)CODE/Logicytics.py(9 hunks)CODE/config.ini(1 hunks)CODE/dump_memory.py(5 hunks)CODE/logicytics/Config.py(2 hunks)CODE/logicytics/Flag.py(2 hunks)CODE/logicytics/Get.py(2 hunks)CODE/logicytics/Logger.py(9 hunks)CODE/logicytics/__init__.py(1 hunks)CODE/network_psutil.py(4 hunks)CODE/packet_sniffer.py(2 hunks)CODE/vulnscan.py(0 hunks)PLANS.md(1 hunks)
💤 Files with no reviewable changes (1)
- CODE/vulnscan.py
✅ Files skipped from review due to trivial changes (1)
- .idea/csv-editor.xml
🧰 Additional context used
🪛 Ruff (0.8.2)
CODE/logicytics/Logger.py
206-206: f-string without any placeholders
Remove extraneous f prefix
(F541)
CODE/logicytics/__init__.py
5-5: logicytics.Config.VERSION imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: logicytics.Config.CURRENT_FILES imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: logicytics.Config.DELETE_LOGS imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: logicytics.Config.config imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (45)
PLANS.md (1)
11-11: Version update looks good!Just a heads up, you've bumped the version for adding docstrings from v3.4.1 to v3.4.2. Makes total sense to move this task to the next version since you're busy fixing those performance bugs in this PR.
CODE/logicytics/__init__.py (1)
5-5:Details
✅ Verification successful
Variable renaming looks good, but check for unused imports
Nice job switching from
CONFIGto lowercaseconfig- that makes naming way more consistent across the codebase. But heads up, the static analysis tool is flaggingVERSION,CURRENT_FILES,DELETE_LOGS, andconfigas possibly unused imports. Double-check if you actually need these or if they can be removed.
🏁 Script executed:
#!/bin/bash # Check if these imported variables are actually used in the codebase echo "Looking for usages of VERSION, CURRENT_FILES, DELETE_LOGS, and config..." rg "VERSION" --type py ./CODE rg "CURRENT_FILES" --type py ./CODE rg "DELETE_LOGS" --type py ./CODE rg "config\." --type py ./CODE # Look for config with dot to find attribute accessLength of output: 7983
Good news – the unused import warning seems to be a false alarm!
Your renaming to
configis totally on point, and after checking, here's what I found:
VERSIONis cited in_debug.py,_dev.py, and elsewhere.CURRENT_FILESis used in_dev.py.DELETE_LOGSappears inLogicytics.py.configis actively utilized in many files, with several attribute accesses noted.So, no changes are needed on these imports. Keep up the great work!
🧰 Tools
🪛 Ruff (0.8.2)
5-5:
logicytics.Config.VERSIONimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
5-5:
logicytics.Config.CURRENT_FILESimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
5-5:
logicytics.Config.DELETE_LOGSimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
5-5:
logicytics.Config.configimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
.gitignore (1)
325-330: Good exclusion of SysInternal executablesSmart move adding these SysInternal Suite executables to the gitignore! Binary files like these shouldn't be tracked in version control, and this helps prevent accidental commits. This change aligns perfectly with your PR objectives to improve directory management.
CODE/logicytics/Config.py (5)
18-18: Documentation update for consistencyYou've updated the docstring to match the new variable name - nice attention to detail!
33-35: Local variable naming improvementRenaming the local variable to
config_localmakes it super clear that this is a local instance of the config parser. Good practice to avoid confusion with the globalconfigvariable.
37-41: Updated config variable referencesThese changes consistently use the new
config_localvariable name. This matches your PR goal of unifying config object usage throughout the codebase.
44-44: Updated return valueYou're now correctly returning the newly named
config_localvariable. This change is consistent with the other renaming changes in this file.
50-50: Global variable renamingYou've changed the exported variable from uppercase
CONFIGto lowercaseconfig. This aligns with Python naming conventions where constants are typically UPPERCASE and regular variables are lowercase. This change improves consistency across the codebase as mentioned in your PR objectives.CODE/logicytics/Flag.py (4)
11-11: Good update to use lowercase config!Switching from
CONFIGtoconfigmakes the code more consistent with modern Python naming conventions. This is a really nice change!
18-20: Nice consistency update!Updating these config object references to match the new import name is solid practice. Makes the code easier to follow for everyone.
24-27: Smart threshold validation update!Changing the acceptable range for
MIN_ACCURACY_THRESHOLDfrom 0-100 to 1-99 makes more sense. A 0% accuracy threshold would match everything, and a 100% threshold would match nothing, so this is a much better constraint.
63-69: Good config reference update!Updated the SentenceTransformer model name retrieval to use the new lowercase
configobject. Also nice error handling that shows the actual model name that might be invalid.CODE/logicytics/Logger.py (4)
6-6: Added regex import for ANSI code removalGood addition of the regex module that'll be used to clean up log messages.
103-107: Updated to use renamed _raw methodProperly updated the method call to use the private
_rawmethod instead of the now-renamedrawmethod.
179-212: Great improvement to logging with ANSI code removal!Renaming
rawto_rawbetter indicates this is an internal method. The big win here is adding regex to remove ANSI escape sequences from logs, which means your log files will be much cleaner and easier to read!Also nice improvement on checking if the frame exists before accessing it.
🧰 Tools
🪛 Ruff (0.8.2)
206-206: f-string without any placeholders
Remove extraneous
fprefix(F541)
245-245: Updated all raw method callsProperly updated all the calls to use the renamed
_rawmethod. This ensures consistency throughout the codebase.Also applies to: 255-255, 265-265, 275-275, 327-328
CODE/logicytics/Get.py (1)
14-14: Super useful directory exclusion feature!Adding the
exclude_dirsparameter is a great improvement! This lets users skip entire directories when searching for files, which can really speed things up and prevent errors when trying to process files from directories that shouldn't be included.The implementation is clean and efficient:
- The parameter is clearly documented
- You convert it to a set for faster lookups
- You modify the
dirslist in-place to preventos.walkfrom even entering those directoriesThis will be really helpful for avoiding problematic directories like those mentioned in the PR description.
Also applies to: 25-25, 33-34, 38-43
CODE/dump_memory.py (5)
8-8: Good config import additionUpdated the imports to include the new
configobject that will be used for dynamic settings.
14-18: Great dynamic config with error handling!Using configuration values for
LIMIT_FILE_SIZEandSAFETY_MARGINinstead of hardcoded values is a big improvement. The validation check forSAFETY_MARGINensures the program won't break with invalid config values.
45-45: Much better file organization!Moving the output files to a dedicated
memory_dumpsdirectory is a great improvement. It keeps everything organized and prevents cluttering the main directory.Also applies to: 95-95, 131-131
136-136: Dynamic safety margin for disk spaceUsing the configurable
SAFETY_MARGINinstead of the hardcoded 2x factor gives users more control over the memory dump behavior.
210-210: Directory creation ensures the code won't breakAdding
os.makedirswithexist_ok=Trueis a smart addition that ensures the output directory exists before trying to write files. This prevents file write errors when the directory doesn't exist.CODE/config.ini (5)
71-72: No real content here.
73-82: NetworkPsutil config looks cool!
The newsample_countandintervalsettings are neat. Should help measure network usage more accurately!
83-84: No real content here.
90-92: Heads up for lowering packet_count to 5000.
Just a quick note: by halving it from 10k to 5k, you might catch fewer packets. Double-check if that's what you really want.
59-70:Details
❓ Verification inconclusive
Be sure “0” truly means no limit!
Love that you can switch off the file size limit by setting it to 0. Let’s quickly confirm thatdump_memory.pywon’t freak out when this value is zero.Try this script to see how
file_size_limitis handled indump_memory.py:
🏁 Script executed:
#!/bin/bash fd dump_memory.py --exec cat {} | rg "file_size_limit"Length of output: 147
Double-check 0 (no limit) handling in dump_memory.py
I verified that the configuration is read correctly—
dump_memory.pyusesconfig.getint("DumpMemory Settings", "file_size_limit")to setLIMIT_FILE_SIZE, meaning that when you set it to 0 inconfig.ini, it will indeed be read as 0 (i.e., "no limit"). However, please make sure that the rest of the logic indump_memory.pyproperly treats 0 as disabling the file size limit (for example, by conditionally skipping any truncation based on a check likeif LIMIT_FILE_SIZE > 0).
- Location:
CODE/config.ini(Lines 59‑70)- Usage in
dump_memory.pywhereLIMIT_FILE_SIZEis assigned.If you're confident that the 0 value is handled correctly throughout the file dumping process, then everything looks good. Otherwise, consider adding or double-checking the specific condition that safely bypasses file size checks when the limit is 0.
CODE/network_psutil.py (5)
7-7: Importing config from logicytics is smooth.
No issues here—awesome job.
15-17: Constructor is nicely updated with config usage.
StoringSAMPLE_COUNTandINTERVALfrom the file is a rad approach—keeps everything more flexible.
20-20: Going async is sweet!
Turninggetinto an async method helps you do concurrency like a pro.
31-31: Awaits the internal bandwidth check—nice.
This cleanly ties in your new async approach. Great job.
190-195: Cool use of asyncio in main block.
This try/except is a brilliant way to catch cancellations or other errors.CODE/packet_sniffer.py (2)
11-11: Import line is all good.
No concerns here.
356-359: Reading config directly is neat.
This makes the code more consistent. Awesome improvement.CODE/Logicytics.py (11)
15-15: Good job updating the imports!You've updated the import statement to use
configinstead ofCONFIG, which is consistent with the changes across other files to standardize the config variable name.
20-20: Nice config update!Updated config usage to match the import change, keeping things consistent.
70-71: Smart move excluding those directories!Adding
exclude_dirs=["logicytics", "SysInternal_Suite"]prevents the script from trying to execute files in these directories, which should fix the execution errors mentioned in the PR objectives.
105-106: Consistent directory exclusion - good stuff!You've applied the same directory exclusion to the MODS directory scan, which keeps the behavior consistent.
133-133: Helpful debug logging!Adding this debug log for execution list length makes troubleshooting easier. This aligns with the PR objective to improve logging.
214-215: Smart fix for memory delta reporting!Clamping negative memory deltas to zero using
max(0, end_memory - start_memory)prevents confusing negative memory usage reports. This addresses theValueErrorrelated to string formatting mentioned in the PR objectives.
218-226: Great error handling upgrade!You've added a try-except block and improved the memory logging with better formatting and error handling. The increased decimal precision (from 2 to 3) also provides more accurate memory reporting.
I especially like how you added a warning about possible interference from external processes when negative memory deltas are detected.
231-239: Smart defensive programming!Adding specific exception handling for both
StopIterationand generic exceptions makes the memory reporting more robust. Now it won't crash when memory data is missing or calculation fails.
248-250: Better user explanations - nice!The improved messaging in the performance summary helps users understand what's happening with memory measurements and sets appropriate expectations about accuracy.
454-476: Great OOP refactoring!Converting the
zip_generated_files()function to aZIPclass with class methods is a good move toward better code organization. The class structure makes the code more maintainable and follows better OOP principles.The
@classmethodand@staticmethoddecorators are used correctly here.
522-522: Consistent method update!Updated to use
ZIP.files()to match the new class structure. This change maintains consistency with the refactoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CODE/_dev.py (1)
119-119: Nice add! Excluding that SysInternal_Suite directory is smart.This change makes total sense - you're adding the
exclude_dirsparameter to skip the "SysInternal_Suite" directory during file listing. This matches what the PR is trying to fix and should prevent those execution errors you mentioned.One small thing though - maybe consider moving "SysInternal_Suite" to a constant at the top of the file like you did with
EXCLUDE_FILES? That way if you need to change it later, you only have to update it in one place.- files = Get.list_of_files(".", exclude_files=EXCLUDE_FILES, exclude_dirs=["SysInternal_Suite"]) + EXCLUDE_DIRS = ["SysInternal_Suite"] + files = Get.list_of_files(".", exclude_files=EXCLUDE_FILES, exclude_dirs=EXCLUDE_DIRS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CODE/_dev.py(1 hunks)CODE/config.ini(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CODE/config.ini
|
Code Climate has analyzed commit efbc0a2 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Pull Request Template
Prerequisites
--devflag, if required.PR Type
update
Description
Fixed major bugs that affect Logicytics (performance checker) and added QoL changes
Logicytics.py
Fixed bug [ValueError String Formatting], where the memory was not a proper string if an application runs into an error
Thanks to new changes,
Get.list_of_filesnow ignores thelogicyticsfolder and, SysInternal_Suitefolder, which resulted in bugs when executed and crashesAdded debug log to get length of execution list
Hard limited memory changes (in performance checker) to be at minimum 0 (Logging will still show you the warning), this is to exclude the edge cases where freed memory is more than used memory
Improved logging of
performance checkwhere messages are more in detail and memory is now in 3dp rather than 2dpAdded try-except statements to prevent crashes if code fails (performance checker)
Made function
zip_generated_files()into a classZIP, with the main and helper function in itGet.py
Fixed bug in execution list where it included the
logicyticslibrary, which led to them being run, thus errors, adding new param:exclude_dirsFlag.py
Fixed bug where CONFIG wasn't importing due to improper import statement (
from .Config import CONFIGis nowfrom logicytics.Config import CONFIG)network_psutil.py
Fixed bug where __measure_network_bandwidth_usage wasn't run due to faulty async-await code, now fixed by making
getand async function, and awaits__measure_network_bandwidth_usageImproved error logging to prevent crashes.
Also, gitignore now ignore the exe files that are zipped when they are unzipped (
SysInternal suite), Thus developers don't accidentally commit themSigned-off-by: Shahm Najeeb [email protected]
This pull request includes multiple changes to the
CODEdirectory, focusing on improving configuration management, enhancing logging and performance tracking, and restructuring some utility functions. The most important changes are summarized below:Configuration Management Improvements:
configobject instead of individualCONFIGimports, improving consistency and maintainability (CODE/Logicytics.py,CODE/dump_memory.py,CODE/logicytics/Config.py,CODE/logicytics/Flag.py). [1] [2] [3] [4]Logging and Performance Tracking Enhancements:
CODE/Logicytics.py). [1] [2]CODE/Logicytics.py). [1] [2]Utility Function Restructuring:
ZIPclass to encapsulate file zipping functionality, replacing the previous standalone function (CODE/Logicytics.py). [1] [2]Configuration File Updates:
config.inifile for memory dumping and network usage measurement, providing more control over these features (CODE/config.ini).Directory and File Management:
Get.list_of_filesfunction to support excluding specific directories, improving file selection flexibility (CODE/logicytics/Get.py). [1] [2]Motivation and Context
Solved many bugs that stopped Logicytics from working, as well as improve flexibility of user inputs and settings
Credit
N/A
Issues Fixed
N/A
Summary by CodeRabbit