#634 Logging #660

HadesArchitect · 2024-12-22T20:13:18Z

Fixes #634: "Replace custom logger with a standard library."

What was done

Removed in-house logger and related files
Introduced new dependency electron-log
Replaced usage of in-house logger with electron-log
Added more logs to have better visibility of the app workflows
Replaced generic log messages (e.g., 'workspaces') with more specific ones ('Failed to save workspace')
Added logs to the catch blocks that were empty before to avoid error suppression catch (e) {} Maintainability Improvement: avoid error suppression #710

Refactored a few usages of exceptions for the flow control to avoid logging zeroes
Added more details about logged events, improving clarity and preparing logs for automated processing, if it will be introduced later.

log.warning('Failed to move workspace', 
    {'workspace': workspace.name, 'old_path': originalPath, 'new_path': newPath, 'error': e})

How to test

Due to a lack of automated testing and a big changeset (25 files affected, +570/-1300), we must run extensive manual testing to avoid regression.

Fetch the code and switch the branch to ha/#634-logging
Execute npm i to get the new dependency
Run the app to see logs in the console. They appear in the DevTools console as well as in the IDE console. I recommend using VSCode with a preconfigured main + renderer debugger.

Review the logs and check if they are correctly describing the app state and processes.
Enable logging in the configuration and restart the app. A new logfile must appear. The location of the file is OS specific, for OSX it's /Users/[USER]/Library/Logs/AxonOps Workbench/main.log Path to the log file is printed in console if the logging to file is enabled.

Disable logging in the configuration and restart the app. New logs must not appear in the log file.
Run through the app features and user scenarios to test if any regression was introduced. (I did it myself but one is not enough)
Review the changeset. I'm very open for the discussion and any suggestions.

Special notes

I've updated the English localization file to specify logging to the file, but I can't do it for other languages.
As I ran out of available time, some desired features (e.g., configurable LOGLEVEL) haven't been implemented. If those are important/desired, a new ticket must be created.
For non-info events (errors), LOGLEVEL was chosen based on the error handling in the code. As most errors are suppressed and control flow doesn't change, accordingly a warning log-level has been set. Personally, I disagree with many of those cases as I'd consider them as errors, thus error log-level would be more suitable, but they aren't processed as errors (control flow goes as if nothing happened).

# main/main.js
  ...
  // Load the main HTML file of that window
  try {
    windowObject.loadURL(URL.format({
      pathname: viewPath,
      protocol: 'file:',
      slashes: true
    }))
  } catch (e) {
    log.warning('Failed to load view file', {'path': viewPath, 'error': e})
    // --->>> TODO: it's an exceptional situation, must be error+abort, not warning <<<---
  }
  
  // Whether or not the window should be at the center of the screen
  if (extraProperties.center)
  ...

Using Exceptions to by-pass parts of code (throw 0) is an anti-pattern. Exceptions are designed for handling exceptional situations, not a regular flow control (https://wiki.c2.com/?DontUseExceptionsForFlowControl).
- In the workbench, those zeroes are used widely (almost 0.5k cases), so they inevitably will appear in logs
- As it wasn't the primary goal of the ticket, I mostly ignored it and only refactored a couple of places to demonstrate that the code looks better without them. This screenshot shows how we avoid anti-pattern and decrease line count, making the app less complex -> improving maintainability.

HadesArchitect · 2024-12-22T20:13:51Z

Work in Progress

millerjp · 2025-01-01T16:09:20Z

@mhmdkrmabd @HadesArchitect how we doing on this? Do we want to pull this into the next major release (right click)?

HadesArchitect · 2025-01-01T17:02:14Z

It looks good and will be finalized tomorrow, but I'm worried about proper testing. With no autotests and a huge changeset, we'll need to test everything manually.

HadesArchitect · 2025-01-13T18:08:24Z

loaded event is never fired on my side, not sure why. Will investigate tomorrow.

HadesArchitect · 2025-01-14T18:43:46Z

With more debug added, I see why the app is never loaded. Reason is, pty:cqlsh:initialize:finished is never fired because cqlsh fails to run.

It brings two questions:

Why does it fail?
How do we detect an issue like that so we write something meaningful to user and in log?

@mhmdkrmabd @digiserg, wdyt?

19:24:29.371 › Renderer is loaded
19:24:29.585 › EVENT pty:cqlsh:initialize
19:24:29.586 › Initializing CQLSH...
19:24:29.585 › Getting the key for encryption or decryption... type private
19:24:29.603 › PTY instance returned data [2945] Failed to load Python shared library '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python': dlopen: dlopen(/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python, 0x000A): tried: '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file), '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file)
...
19:24:31.647 › Waiting initialization... Loaded: true Initialized: false
19:24:32.550 › Waiting initialization... Loaded: true Initialized: false
19:24:33.456 › Waiting initialization... Loaded: true Initialized: false
19:24:34.363 › Waiting initialization... Loaded: true Initialized: false

Much later, after I manually exit the workbench:

...
19:25:57.638 › PTY instance exited! exitcode: 0 signal: 9

Some log output is omitted for clarity

HadesArchitect · 2025-01-14T18:47:47Z

Manual call for cqlsh gives the same error:

aleks@aleksmacbookcable axonops-workbench % ./main/bin/cqlsh/cqlsh 
[3428] Failed to load Python shared library '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python': dlopen: dlopen(/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python, 0x000A): tried: '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file), '/Users/aleks/axonops-workbench/main/bin/cqlsh/_internal/Python' (no such file)
aleks@aleksmacbookcable axonops-workbench % echo $?
255

The cqlsh file was downloaded as described in instructions

mhmdkrmabd · 2025-01-14T18:52:22Z

@HadesArchitect well based on the log it seems the building process of cqlsh wasn't successful, a Python shared library is missing, and the workbench - and this is only happening on macOS - is running an initialization process to grant privileges regarding the access of OS keychain and other permissions. The workbench would show notification to users about missing binaries, but in our case it's seems to be corrupted.

Did you build the binaries from your side? If so you please try those instead:
https://github.com/axonops/axonops-workbench-cqlsh/releases/tag/0.14.3

P.S: Saw the new comment. Let us test the darwin version of the binaries in our side to make sure it's not a common issue

HadesArchitect · 2025-01-14T18:54:24Z

Thanks @mhmdkrmabd I'll try those tomorrow

digiserg · 2025-01-14T20:49:22Z

Use this script to download the binaries:

          CQLSH_BUILD_VERSION="0.14.3"
          CQLSH_GITHUB_URL='https://github.com/axonops/axonops-workbench-cqlsh/releases/download'

          mkdir -p main/bin
          for binary in cqlsh keys_generator; do
            curl -fL ${CQLSH_GITHUB_URL}/${CQLSH_BUILD_VERSION}/${binary}-$(uname -s)-$(uname -m).tar | tar xf - -C main/bin
            mv main/bin/${binary}-$(uname -s)-$(uname -m) main/bin/${binary}
            mv main/bin/${binary}/${binary}-$(uname -s)-$(uname -m) main/bin/${binary}/${binary}
          done

HadesArchitect · 2025-01-15T11:03:03Z

Thanks, @digiserg. This script works. I see it brings more than just two files; I'll update the documentation accordingly.

The first question is answered, the second one is still to do: "How do we detect an issue like that so we write something meaningful to user and in log?" I'd extend check of these dependencies: not only check the presence of the files, but maybe call them and check exit code.

HadesArchitect · 2025-02-01T15:38:55Z

Ready for review

mhmdkrmabd · 2025-02-01T15:54:57Z

@HadesArchitect thank you for your effort! I'll review it as soon as possible 👍

HadesArchitect · 2025-02-01T17:04:37Z

Sample log file: main.log

HadesArchitect · 2025-02-03T11:23:52Z

Updated PR description to describe changes, test process and add special notes.

millerjp · 2025-02-19T05:36:56Z

@mhmdkrmabd can you review this please we should merge this in if possible for next weeks release

digiserg · 2025-02-19T09:20:33Z

@mhmdkrmabd @HadesArchitect if you can please fix the merge conflict, I'll trigger a build for this branch to test and it contains lots of changes.

mhmdkrmabd · 2025-02-19T09:23:12Z

@digiserg if @HadesArchitect wasn't have time or not able to do that I'll try to handle it today once I finish work in hand

We've many upcoming PRs, one on the way shortly

HadesArchitect · 2025-02-19T09:31:05Z

I can do it in a couple of hours ср, 19 февр. 2025 г., 10:23 Mohammed Abed El Hay Abu Baker < ***@***.***>:

…

@digiserg <https://github.com/digiserg> if @HadesArchitect <https://github.com/HadesArchitect> wasn't have time or not able to do that I'll try to handle it today once I finish work in hand — Reply to this email directly, view it on GitHub <#660 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANJLXJBTKGMCQH3QIUUXTL2QRERTAVCNFSM6AAAAABUB6MUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRYGAZDOMZZGU> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: mhmdkrmabd]*mhmdkrmabd* left a comment (axonops/axonops-workbench#660) <#660 (comment)> @digiserg <https://github.com/digiserg> if @HadesArchitect <https://github.com/HadesArchitect> wasn't have time or not able to do that I'll try to handle it today once I finish work in hand — Reply to this email directly, view it on GitHub <#660 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANJLXJBTKGMCQH3QIUUXTL2QRERTAVCNFSM6AAAAABUB6MUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRYGAZDOMZZGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Signed-off-by: Aleks Volochnev <a.volochnev@gmail.com>

HadesArchitect · 2025-02-19T20:59:24Z

Resolved conflict

mhmdkrmabd · 2025-02-19T21:15:36Z

custom_node_modules/renderer/clusters.js

-
-          // Add those secrets to the final result
-          temp.info.secrets = JSON.parse(secret.password)
+          if (secret == undefined) {


This needs to be secret != undefined

mhmdkrmabd · 2025-02-19T21:18:16Z

custom_node_modules/renderer/clusters.js

-      )
-    } catch (e) {
+    // If no secrets have been provided then skip
+    if (cluster.info.secrets != undefined && Object.keys(cluster.info.secrets).length > 0) {


This needs to be back within try-catch block, in case cluster.info doesn't have secrets attribute/key it'll throw a critical runtime error

mhmdkrmabd · 2025-02-19T21:20:01Z

custom_node_modules/renderer/docker.js


    // Get a random free-to-use ports for the AxonOps agent and Apache Cassandra
-    [this.axonopsPort, this.cassandraPort] = await getRandomPort(2)


No need for this change, getRandomPort function return an array of ports

mhmdkrmabd · 2025-02-19T21:28:41Z

renderer/js/background_processes.js

-              authentication.privateKey = prasedKey.getPrivatePEM()
-            } catch (e) {
+            // If a pass phrase for the private key file has been passed
+            if (data.passphrase != undefined && data.passphrase.trim().length > 0) {


This line of code needs to be within a try-catch block, trim function will throw an error in case it's called by non-String object, in case where data.passphrase is undefined a runtime error will cause the process to be terminated

mhmdkrmabd · 2025-02-19T21:32:59Z

renderer/js/background_processes.js

-            if (sshTunnel.port != clusterID)
-              return
-
+          if (isPort) {


This part needs to be reviewed, looks like it return different results from the original implementation

mhmdkrmabd · 2025-02-19T21:36:15Z

renderer/js/events/common.js

-                errorLog(e, 'common')
-              } catch (e) {}
+                // Save `username`
+                if (credentialsArray.authusername.trim().length != 0)


This should be within try-catch block, reason explained in a previous comment

mhmdkrmabd · 2025-02-19T21:36:29Z

renderer/js/events/common.js

-                errorLog(e, 'common')
-              } catch (e) {}
+                // Save the SSH `username`
+                if (credentialsArray.sshusername.trim().length != 0)


This one also

mhmdkrmabd · 2025-02-19T21:48:32Z

Okay @HadesArchitect thanks a lot for the effort and time you did for that PR. I've added some comments that should be handled, when they're handled I'll consider to do further tests. Regarding the try-catch blocks, if statements, the way of handling things, everything is in place for a purpose, it's fine to modify minor things - like using find instead of filter, thanks for catching those in old files 🙌 - but ensuring the same output is important.

Please consider reading the comments when you have time, am tuned for the changes, thank you! 😄

millerjp · 2025-03-04T10:49:56Z

@mhmdkrmabd @HadesArchitect any updates on this?

HadesArchitect · 2025-03-04T11:17:53Z

Sorry, guys, the review got me right on a vacation. I'll try to make requested changes in the next few days.

HadesArchitect added 10 commits December 13, 2024 14:36

Added VSCode Debugger Configuration

8d12a24

electron-log demo

d7734e2

Merge branch 'axonops:main' into main

b520d51

electron-log demo renderer

b887b25

More details on logging proposal

f0791ad

Returned removed statement

f9b329d

Clean up

f61b7e1

Clean up

04600c1

Added electron-log dependency

399f3aa

axonops#634 First approach

543d664

axonops#634 Second Approach

97d6a3a

HadesArchitect self-assigned this Dec 23, 2024

HadesArchitect added 4 commits January 2, 2025 21:35

axonops#634 Third approach

102d890

Merge remote-tracking branch 'upstream/main'

0a027ea

Updated to latest main

0d2246a

axonops#634 Improvements and more debug

332347e

axonops#634 More debug in pty module

78e2080

HadesArchitect added 3 commits January 21, 2025 21:13

axonops#634 Meaningful log messages and info

e671c4d

axonops#634 Meaningful log messages, little refactoring

fed3d95

axonops#634 Meaningful logs, log data

7110ce6

HadesArchitect requested a review from mhmdkrmabd February 1, 2025 15:38

Merge branch 'main' into axonops#634-logging

361fc7a

HadesArchitect changed the title ~~[WiP] #634 Logging~~ #634 Logging Feb 3, 2025

HadesArchitect added the Refactoring label Feb 3, 2025

Merge branch 'main' into axonops#634-logging

3141327

millerjp removed request for digiserg and millerjp February 19, 2025 05:41

millerjp added the right click label Feb 19, 2025

millerjp added this to the RightClick milestone Feb 19, 2025

Merge branch 'main' into axonops#634-logging

b00bab7

Signed-off-by: Aleks Volochnev <a.volochnev@gmail.com>

mhmdkrmabd reviewed Feb 19, 2025

View reviewed changes

millerjp marked this pull request as draft February 24, 2025 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#634 Logging #660

#634 Logging #660

HadesArchitect commented Dec 22, 2024 •

edited

Loading

HadesArchitect commented Dec 22, 2024

millerjp commented Jan 1, 2025

HadesArchitect commented Jan 1, 2025

HadesArchitect commented Jan 13, 2025

HadesArchitect commented Jan 14, 2025 •

edited

Loading

HadesArchitect commented Jan 14, 2025 •

edited

Loading

mhmdkrmabd commented Jan 14, 2025 •

edited

Loading

HadesArchitect commented Jan 14, 2025

digiserg commented Jan 14, 2025

HadesArchitect commented Jan 15, 2025

HadesArchitect commented Feb 1, 2025

mhmdkrmabd commented Feb 1, 2025

HadesArchitect commented Feb 1, 2025

HadesArchitect commented Feb 3, 2025

millerjp commented Feb 19, 2025

digiserg commented Feb 19, 2025

mhmdkrmabd commented Feb 19, 2025 •

edited

Loading

HadesArchitect commented Feb 19, 2025 via email

HadesArchitect commented Feb 19, 2025

mhmdkrmabd Feb 19, 2025

mhmdkrmabd Feb 19, 2025 •

edited

Loading

mhmdkrmabd Feb 19, 2025

mhmdkrmabd Feb 19, 2025

mhmdkrmabd Feb 19, 2025

mhmdkrmabd Feb 19, 2025

mhmdkrmabd Feb 19, 2025

mhmdkrmabd commented Feb 19, 2025

millerjp commented Mar 4, 2025

HadesArchitect commented Mar 4, 2025


		// Get a random free-to-use ports for the AxonOps agent and Apache Cassandra
		[this.axonopsPort, this.cassandraPort] = await getRandomPort(2)

#634 Logging #660

Are you sure you want to change the base?

#634 Logging #660

Conversation

HadesArchitect commented Dec 22, 2024 • edited Loading

What was done

How to test

Special notes

HadesArchitect commented Dec 22, 2024

millerjp commented Jan 1, 2025

HadesArchitect commented Jan 1, 2025

HadesArchitect commented Jan 13, 2025

HadesArchitect commented Jan 14, 2025 • edited Loading

HadesArchitect commented Jan 14, 2025 • edited Loading

mhmdkrmabd commented Jan 14, 2025 • edited Loading

HadesArchitect commented Jan 14, 2025

digiserg commented Jan 14, 2025

HadesArchitect commented Jan 15, 2025

HadesArchitect commented Feb 1, 2025

mhmdkrmabd commented Feb 1, 2025

HadesArchitect commented Feb 1, 2025

HadesArchitect commented Feb 3, 2025

millerjp commented Feb 19, 2025

digiserg commented Feb 19, 2025

mhmdkrmabd commented Feb 19, 2025 • edited Loading

HadesArchitect commented Feb 19, 2025 via email

HadesArchitect commented Feb 19, 2025

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd Feb 19, 2025

Choose a reason for hiding this comment

mhmdkrmabd commented Feb 19, 2025

millerjp commented Mar 4, 2025

HadesArchitect commented Mar 4, 2025

HadesArchitect commented Dec 22, 2024 •

edited

Loading

HadesArchitect commented Jan 14, 2025 •

edited

Loading

HadesArchitect commented Jan 14, 2025 •

edited

Loading

mhmdkrmabd commented Jan 14, 2025 •

edited

Loading

mhmdkrmabd commented Feb 19, 2025 •

edited

Loading

mhmdkrmabd Feb 19, 2025 •

edited

Loading