feat: Adds extension name as prefix when extension is using logger #872

sfrunza13 · 2022-11-20T23:04:00Z

What does this PR do?

I am aiming to create a wrapper for some of the logging so that we can have a prefix for each separate extension.

Screenshot/screencast of this PR

Something similar to this

What issues does this PR fix or reference?

#822

How to test this PR?

Check logs originating from extensions folder for the appropriate names

Signed-off-by: Stefan Frunza stefanfrunza@gmail.com

sfrunza13 · 2022-11-20T23:08:43Z

@benoitf I apologize as I am still relatively new to everything. This is where I am at right now, I thought it would be more productive to open a PR review and ask for some guidance on how to get it right rather than continue shooting in the dark as it were.

Am I on the right track?

benoitf · 2022-11-21T09:16:34Z

Helo @sfrunza13
So the overall idea is that extension does not care about the prefix. Extension should be 'dumb' and just use console.log messages.

It's inside the 'patched' console.log method that we should be 'smarter' and see if it's an extension that is calling us or not.
If yes, we prefix using the extension name, else we display the log as usual.

https://github.com/containers/podman-desktop/blob/df9172a9218366f26fad7b5ee7fb8d9933bba286/packages/main/src/plugin/index.ts#L125-L130

so probably, look at the stack trace, if stack is coming from an extension, grab the name
and we should have a map between the folder name and the extension name so we display the extension name

we probably need to retrieve some values from extensionLoader class (like the name of the root extension folder or all extension folders used

sfrunza13 · 2022-11-21T21:06:58Z

@benoitf how is this? What would you advise to improve it?

benoitf · 2022-11-21T21:31:56Z

@sfrunza13 I would not hard code the list of possible extensions

I would ask extensionLoader to grab the current root directory for extensions or the set of directories that are currently loaded

benoitf · 2022-11-21T21:32:37Z

but it looks very promising 🎉 thanks for looking at it

benoitf · 2022-11-21T21:34:18Z

And FYI, to get a stack, no need to throw and catch

const stack = new Error().stack

benoitf · 2022-11-21T21:36:41Z

packages/main/src/plugin/index.ts

+    }
+
+    if(stack!=''){
+      var extensionGroupings = stack.match(/(podman\-desktop\\extensions)\\(lima|crc|docker|kube\-context|podman)\\(dist\\extension\.js)+:/)


note that it could be other JavaScript object than the entrypoint and it's not forced to be named extension.js

So here probably we should just check if parent folder is part of an extension

sfrunza13 · 2022-11-22T04:17:42Z

Currently, I can think of 2 ways of going about it, both of which involve creating a private variable in index.ts to have the extension ids in a scope I can work with. Either have my knowledge of the extension ids tied to extensionLoader.start() by retrieving them with .listExtensions(), in which case I can not append the prefix to logs that occur before this, or import path to index.ts and duplicate the read logic found within .start() right after the creation of the extensionLoader.

Are either of these acceptable? I am certain I am missing a better solution.

benoitf · 2022-11-22T09:35:11Z

Currently, I can think of 2 ways of going about it, both of which involve creating a private variable in index.ts to have the extension ids in a scope I can work with. Either have my knowledge of the extension ids tied to extensionLoader.start() by retrieving them with .listExtensions(), in which case I can not append the prefix to logs that occur before this, or import path to index.ts and duplicate the read logic found within .start() right after the creation of the extensionLoader.

Are either of these acceptable? I am certain I am missing a better solution.

Hello @sfrunza13

We want to prefix the logging for extensions so it's ok to wait that extensionLoader is created to add a reference
Extensions won't be started during that time.

packages/main/src/plugin/index.ts
// redirect main process logs to the extension loader
this.redirectLogging();

const extensionLoader = new ExtensionLoader(
(add extensionLoader as a private variable)

then in redirectConsole() method you'll access to extensionLoader

from there (if extensionLoader is not defined, we don't try to add a prefix
if it's there, then listExtensions (as you suggested can be used) or we can add another method that returns a mapping between extensionName and root folders or the opposite

So redirectConsole should have all elements before an extension is started.

sfrunza13 · 2022-11-22T16:14:01Z

@benoitf I tried using listExtensions() but the analyzedExtensions member that it is trying to return do not seem to be populated until after the extensionLoader.start() method and not the extensionLoader creation. So what I have done is I factored out the extension file reading from the start() method and I used that to build my list of extensionNames to check against. Is this ok?

benoitf · 2022-11-22T17:04:31Z

Hi @sfrunza13 I'm not sure to get all of it. But I would assume that it's ok to only try to call listExtensions only when extensionLoader is defined.

it's ok if it's not populated until the start of extension loader as we populate the list before activating the extensions (and this is where the extension is starting)

sfrunza13 · 2022-11-22T18:33:32Z

@benoitf I think this time I might have got the idea

benoitf · 2022-11-23T07:55:43Z

packages/main/src/plugin/index.ts

+      //Take the id information from extensionLoader
+      let validExtObj = await this.extensionLoader.listExtensions();
+
+      validExtObj.forEach((extInfo: any) => {


AFAIK the type is not any, we have a known type returned by listExtensions()

benoitf · 2022-11-23T07:58:58Z

packages/main/src/plugin/index.ts

+      let validExtObj = await this.extensionLoader.listExtensions();
+
+      validExtObj.forEach((extInfo: any) => {
+        validExt.push(extInfo.id);


I think we should grab the path of the extension (add that information in the info object)
so we don't hardcode some path in the regexp

benoitf · 2022-11-23T08:00:41Z

packages/main/src/plugin/index.ts

+      const re = new RegExp(`(podman\-desktop\\\\extensions)\\\\(${insert})`)
+      var extensionGroupings = stack.match(re)
+      if (extensionGroupings != null)
+      return '[' + extensionGroupings[2] + ']';


we need to display the extension name, not the id, so we should keep the mapping extension path<--> extension name

sfrunza13 · 2022-11-23T20:40:45Z

How's this?

benoitf · 2022-11-24T10:43:00Z

FYI I tested it and it does not work on my computer

main ↪️ Activating extension (crc)
index.ts:106 main ↪️ Can not find CRC binary!
(anonymous) @ index.ts:106
index.ts:104 main ↪️ Activation extension (crc) ended
index.ts:104 main ↪️ Activating extension (lima)
index.ts:113 main ↪️ Could not find podman socket at /Users/benoitf/.lima/podman-lima/sock/podman.sock nor /Users/benoitf/.lima/podman-lima/sock/podman.sock
(anonymous) @ index.ts:113
index.ts:104 main ↪️ Activation extension (lima) ended
index.ts:104 main ↪️ Activating extension (kube-context)
index.ts:104 main ↪️ starting extension kube-context

benoitf · 2022-11-24T10:44:08Z

packages/main/src/plugin/index.ts

+    //Check if index.ts private member extensionLoader initialized
+    //If it is grab listExtensions()
+    if(this.extensionLoader != undefined){
+      let validExtObj = await this.extensionLoader.listExtensions();
+      //start setting path => name
+      validExtObj.forEach((extInfo: ExtensionInfo) => {
+        extensions.set(extInfo.path, extInfo.name)
+      })
+    }


as an improvement, I think we should build objects only if extension list is changing.
It'll cost too many iteration to do that on each log call.

benoitf · 2022-11-24T10:44:43Z

packages/main/src/plugin/index.ts

+        let searchString = path.replace("/","\\") 
+        searchString = searchString.replaceAll("\\", "\\\\")
+        let re = new RegExp(`${searchString}`)


FYI on my computer, it's not working

benoitf · 2022-11-24T19:18:42Z

packages/main/src/plugin/index.ts

+      })
+
+      if (toAppend != '')
+      return '[' + toAppend + ']';


Suggested change

return '[' + toAppend + ']';

return `[${toAppend}]`;

benoitf · 2022-11-24T19:20:30Z

@sfrunza13 you'll need to sign off the commits

And apply linter/formatter
https://github.com/containers/podman-desktop/blob/main/CONTRIBUTING.md#submitting-pull-requests

benoitf · 2022-11-24T19:23:10Z

https://github.com/containers/podman-desktop/blob/main/CONTRIBUTING.md#step-5-code-formatter--linter

benoitf · 2022-11-24T21:16:02Z

thanks for linter and formatter. The DCO check is still need. You may have to squash all your commits into a single one

Check stack for extensions retrieved from extensionloader, then append them to log (podman-desktop#822) Signed-off-by: stefanaz2 <sfrunza@seneca.ca>

benoitf

thanks @sfrunza13 for the contribution 👍

Activating extension (crc)
[crc] Can not find CRC binary!
Activation extension (crc) ended
Activating extension (lima)
[lima] Could not find podman socket at /Users/benoitf/.lima/podman-lima/sock/podman.sock nor /Users/benoitf/.lima/podman-lima/sock/podman.sock

in a next improvement (not this PR), we may reformat the logging of Activating/Deactivating extensions using the same pattern

so instead of

Activating extension (lima)
[lima] Could not find podman socket at /Users/benoitf/.lima/podman-lima/sock/podman.sock nor /Users/benoitf/.lima/podman-lima/sock/podman.sock
Activation extension (lima) ended

[lima] Activating extension...
[lima] Could not find podman socket at /Users/benoitf/.lima/podman-lima/sock/podman.sock nor /Users/benoitf/.lima/podman-lima/sock/podman.sock
[lima] Activation extension succeed

sfrunza13 · 2022-11-25T15:09:41Z

@benoitf Thank you for your patience and guidance

sfrunza13 force-pushed the #822 branch from 8924144 to 039f1a0 Compare November 20, 2022 23:19

benoitf reviewed Nov 21, 2022

View reviewed changes

benoitf changed the title ~~Each extension with a simple wrapper~~ feat: Adds extension name as prefix when extension is using logger Nov 22, 2022

benoitf reviewed Nov 23, 2022

View reviewed changes

benoitf reviewed Nov 24, 2022

View reviewed changes

sfrunza13 force-pushed the #822 branch 2 times, most recently from 71a5c81 to 35136ee Compare November 24, 2022 22:49

Add extension name to log

bdd1db2

Check stack for extensions retrieved from extensionloader, then append them to log (podman-desktop#822) Signed-off-by: stefanaz2 <sfrunza@seneca.ca>

sfrunza13 force-pushed the #822 branch from 35136ee to bdd1db2 Compare November 24, 2022 22:51

sfrunza13 marked this pull request as ready for review November 25, 2022 02:44

sfrunza13 requested review from vzhukovs and cdrage as code owners November 25, 2022 02:44

benoitf approved these changes Nov 25, 2022

View reviewed changes

benoitf merged commit e94ee39 into podman-desktop:main Nov 26, 2022

podman-desktop-bot added this to the 0.10.0 milestone Nov 26, 2022

benoitf mentioned this pull request Dec 12, 2022

logging of extension should contain a prefix #822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds extension name as prefix when extension is using logger #872

feat: Adds extension name as prefix when extension is using logger #872

sfrunza13 commented Nov 20, 2022 •

edited

Loading

sfrunza13 commented Nov 20, 2022

benoitf commented Nov 21, 2022

sfrunza13 commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf Nov 21, 2022

sfrunza13 commented Nov 22, 2022

benoitf commented Nov 22, 2022

sfrunza13 commented Nov 22, 2022

benoitf commented Nov 22, 2022

sfrunza13 commented Nov 22, 2022

benoitf Nov 23, 2022

benoitf Nov 23, 2022

benoitf Nov 23, 2022

sfrunza13 commented Nov 23, 2022

benoitf commented Nov 24, 2022

benoitf Nov 24, 2022

benoitf Nov 24, 2022

benoitf Nov 24, 2022

benoitf commented Nov 24, 2022

benoitf commented Nov 24, 2022

benoitf commented Nov 24, 2022

benoitf left a comment

sfrunza13 commented Nov 25, 2022

feat: Adds extension name as prefix when extension is using logger #872

feat: Adds extension name as prefix when extension is using logger #872

Conversation

sfrunza13 commented Nov 20, 2022 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

sfrunza13 commented Nov 20, 2022

benoitf commented Nov 21, 2022

sfrunza13 commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf commented Nov 21, 2022

benoitf Nov 21, 2022

Choose a reason for hiding this comment

sfrunza13 commented Nov 22, 2022

benoitf commented Nov 22, 2022

sfrunza13 commented Nov 22, 2022

benoitf commented Nov 22, 2022

sfrunza13 commented Nov 22, 2022

benoitf Nov 23, 2022

Choose a reason for hiding this comment

benoitf Nov 23, 2022

Choose a reason for hiding this comment

benoitf Nov 23, 2022

Choose a reason for hiding this comment

sfrunza13 commented Nov 23, 2022

benoitf commented Nov 24, 2022

benoitf Nov 24, 2022

Choose a reason for hiding this comment

benoitf Nov 24, 2022

Choose a reason for hiding this comment

benoitf Nov 24, 2022

Choose a reason for hiding this comment

benoitf commented Nov 24, 2022

benoitf commented Nov 24, 2022

benoitf commented Nov 24, 2022

benoitf left a comment

Choose a reason for hiding this comment

sfrunza13 commented Nov 25, 2022

sfrunza13 commented Nov 20, 2022 •

edited

Loading