Skip to content
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

Zowe Explorer Output Logger #2192

Merged
merged 61 commits into from
Apr 4, 2023
Merged

Zowe Explorer Output Logger #2192

merged 61 commits into from
Apr 4, 2023

Conversation

JillieBeanSim
Copy link
Contributor

@JillieBeanSim JillieBeanSim commented Mar 17, 2023

Proposed changes

Add Zowe Explorer Logger setting with INFO as default, this setting will be most useful for users.
Screen Shot 2023-03-28 at 10 32 28 AM

Added Zowe Explorer Output Channel to also write logs to.
Screen Shot 2023-03-28 at 10 31 53 AM

If Zowe CLI envar ZOWE_APP_LOG_LEVEL is set and different from ZE's default, the user is prompted once and asked if they would like ZE's logger to match the CLI's. After first presentation of message "zowe.cliLoggerSetting.presented": true is added in settings and the user won't be asked again as long as it is in settings.
Screen Shot 2023-03-29 at 7 29 07 AM

Release Notes

Milestone: 2.8.0

Changelog:
ZE:

  • Added a new Zowe Explorer setting, zowe.logger, with a default setting of INFO.
  • Added an output channel, Zowe Explorer, for logging within VS Code's Output view. The log level is set by the new Zowe Explorer setting, zowe.logger.

ZE API:

  • Added Gui.reportProgress that can be used to notify users of action progress in conjunction with the Gui.withProgress call. #2167

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
…orHandling

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim added this to the v2.8.0 milestone Mar 17, 2023
@JillieBeanSim JillieBeanSim self-assigned this Mar 17, 2023
@JillieBeanSim JillieBeanSim linked an issue Mar 17, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Patch coverage: 86.78% and project coverage change: +0.30 🎉

Comparison is base (0b031e0) 91.15% compared to head (2c04788) 91.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2192      +/-   ##
==========================================
+ Coverage   91.15%   91.46%   +0.30%     
==========================================
  Files          89       89              
  Lines        8400     8856     +456     
  Branches     1807     1828      +21     
==========================================
+ Hits         7657     8100     +443     
- Misses        742      755      +13     
  Partials        1        1              
Impacted Files Coverage Δ
packages/zowe-explorer/src/ZoweExplorerExtender.ts 86.81% <50.00%> (+0.29%) ⬆️
...ges/zowe-explorer/src/command/TsoCommandHandler.ts 85.47% <60.00%> (+<0.01%) ⬆️
packages/zowe-explorer/src/Profiles.ts 82.35% <70.53%> (+1.56%) ⬆️
packages/zowe-explorer/src/job/actions.ts 89.10% <75.00%> (+0.27%) ⬆️
...ges/zowe-explorer/src/abstract/ZoweTreeProvider.ts 81.01% <79.16%> (-0.47%) ⬇️
.../zowe-explorer/src/abstract/ZoweCommandProvider.ts 94.54% <80.00%> (-1.76%) ⬇️
...ges/zowe-explorer/src/command/MvsCommandHandler.ts 93.60% <80.00%> (+0.21%) ⬆️
packages/zowe-explorer/src/dataset/actions.ts 92.34% <81.66%> (+0.26%) ⬆️
packages/zowe-explorer/src/globals.ts 91.85% <81.81%> (-1.01%) ⬇️
packages/zowe-explorer/src/SpoolProvider.ts 82.35% <83.33%> (+0.21%) ⬆️
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@JillieBeanSim JillieBeanSim marked this pull request as ready for review March 29, 2023 12:07
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@rudyflores
Copy link
Contributor

@JillieBeanSim Thanks for these changes Billie! I wanted to suggest if we wanted to add maybe little descriptions for what each log level does in the dropdown setting, you can set them by doing the following for example:

"zowe.security.secureCredentialManager": {
          "default": "keytar",
          "type": "string",
          "enum": [
            "keytar",
            "kubernetes",
            "off"
          ],
          "markdownEnumDescriptions": [
            "Store Zowe credentials on the local machine with Keytar.",
            "Store Zowe credentials on a kubernetes cluster as secrets.",
            "Disable secure credentials and allow creation of an unsecure profile."
          ],
          "description": "%zowe.security.secureCredentialManager%",
          "scope": "window"
        }

Notice the markdownEnumDescriptions property I believe this will show that when hovering on the setting option which might be nice to inform the user what each option performs :)

Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@phaumer
Copy link
Member

phaumer commented Mar 31, 2023

This is great work and will make logging so much better for developers and end-users. I am not submitting a formal review as I have some general feedback that is more an opinion that we need to discuss as a team:

  1. Move to API layer: In our compliance guidelines we have items around common error handling and consistent user experience. I think logging should be part of that. Once configured the user does not really care anymore if they use z/OSMF or FTP to execute Zowe Explorer operations so they would not want to switch around multiple log-files. I would therefore propose to move the logger into the API package and add a recommended compliance item that extensions should use the ZoweLogger.
  2. Replace the dialog: I would redesign that dialog as the default level for logging is already INFO. So when the user clicks the drop-down they already know they want to change the level. Perhaps I miss some information, but should the dialog ask the question the other way round? "Do you want to change the logging level of Zowe logs to be the same?" Because how good of an output can Zowe Explorer provide if the Zowe log messages are from a higher level? Also changing the log level for Zowe CLI is not an easy task, so Zowe Explorer could make it easer for the end user by automating it.
  3. Review log messages and levels: I think this is a good opportunity to define clearly on our wiki what each log level should be used for and review what we log based on that. When I switch to trace I see a lot of output that I don't think is of value to anyone. See the screenshot. At least it should tell me what tree item is being rendered so I could use that to see where the tree building crashes in particular.
  4. Merge in Zowe logs: Other information that is really important is missing, such as the actual REST request submitted to the sever (without confidential info, of course), which I believe is in the Zowe logs, so we need to find a way to pull that into the output, correct? I would like to avoid that users have to go to another place to look at the Zowe logs as they would have to manually merge things in their head using the timestamps. This could be implemented as a log4js Appender to the Zowe CLI log4js logger that writes into the ZoweLogger as well.

2023-03-31_10-57-19

@phaumer
Copy link
Member

phaumer commented Mar 31, 2023

In the VS Code Debug Console I see these messages that look like they are related to the output view.

Notification handler 'window/logMessage' failed with message: Channel has been closed
extensionHostProcess.js:102
rejected promise not handled within 1 second: Error: Channel has been closed
extensionHostProcess.js:102
stack trace: Error: Channel has been closed
	at r (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:109:2578)
	at Object.appendLine (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:109:2696)
	at ZoweLogger.<anonymous> (/Users/phaumer/Dev/Zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/extension.js:195033:85)
	at Generator.next (<anonymous>)
	at fulfilled (/Users/phaumer/Dev/Zowe/vscode-extension-for-zowe/packages/zowe-explorer/out/src/extension.js:194943:58)

zFernand0
zFernand0 previously approved these changes Apr 4, 2023
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

I really like this feature!
LGTM! 😋

packages/zowe-explorer/src/utils/LoggerUtils.ts Outdated Show resolved Hide resolved
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
1.4% 1.4% Duplication

@JillieBeanSim
Copy link
Contributor Author

Comments have been addressed. I created a new Epic for V3 Enhancements that includes a story about exposing the ZE VSC Output Channel logger in ZE API. We already have ZE APIs for writing to the log files and Timothy is working on a new setting for users to state where they would like there logs to be written in this PR that can be utilized by extenders to let users have a single location for them and ZE to write logs to the same files. I also created a story for the wiki update and review of logging
I have pushed changes for the padding of the time and description of the log levels.

@JillieBeanSim JillieBeanSim requested a review from zFernand0 April 4, 2023 19:51
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM! 😋

Copy link
Contributor

@rudyflores rudyflores left a comment

Choose a reason for hiding this comment

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

Thanks for this awesome new change @JillieBeanSim !

@JillieBeanSim JillieBeanSim merged commit 0fd2473 into main Apr 4, 2023
@JillieBeanSim JillieBeanSim deleted the output-logger branch April 4, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log Level User Setting Zowe Explorer Output View logging
4 participants