Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Dec 5, 2024

Problem

We want to only log file paths when in debug mode, but the connect script has no way of determining this.

Solution

  • pass log level to connect script.
  • use it to determine what to log.

Example logs:

2024/12/05 15:50:24 ==============================================================================
2024/12/05 15:50:24 LOG_LEVEL=3
2024/12/05 15:50:24 AWS_REGION=us-east-1
2024/12/05 15:50:24 SESSION_ID=...
2024/12/05 15:51:11 ==============================================================================
2024/12/05 15:51:11 LOG_LEVEL=1
2024/12/05 15:51:11 AWS_REGION=us-east-1
2024/12/05 15:51:11 SESSION_ID=...
2024/12/05 15:51:11 AWS_SSM_CLI=.../Amazon/sessionmanagerplugin/bin/session-manager-plugin
2024/12/05 15:51:11 LOG_FILE_LOCATION=/.../ec2.{instanceId}.log
2024/12/05 15:51:55 ==============================================================================
2024/12/05 15:51:55 LOG_LEVEL=2
2024/12/05 15:51:55 AWS_REGION=us-east-1
2024/12/05 15:51:55 SESSION_ID=default-uk9cv4h86y5rdbrzarlkj9opci
2024/12/05 15:51:56 AWS_SSM_CLI=../Amazon/sessionmanagerplugin/bin/session-manager-plugin
2024/12/05 15:51:56 LOG_FILE_LOCATION=/.../ec2.{instanceId}.log

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Dec 5, 2024

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@Hweinstock Hweinstock changed the base branch from master to feature/postreinvent December 5, 2024 21:42
STREAM_URL: session.StreamUrl,
SESSION_ID: session.SessionId,
TOKEN: session.TokenValue,
LOG_LEVEL: globals.logOutputChannel.logLevel,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like getLogger doesn't have e.g. getLogLevel. otoh, we could make the flag a simple boolean like LOG_DEBUG:

Suggested change
LOG_LEVEL: globals.logOutputChannel.logLevel,
LOG_DEBUG: getLogger().logLevelEnabled('debug') ? '1': '0',

that also would avoid the use of -le in the bash script

Copy link
Contributor Author

@Hweinstock Hweinstock Dec 5, 2024

Choose a reason for hiding this comment

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

I don't think logLevelEnabled works as intended. I was able to produce this:

2024-12-05 17:11:27.838 [info] vscode log level: info
2024-12-05 17:11:27.838 [info] is debug enabled?: true

and I believe its because the internal log level is debug until the user changes it based on the code here:

logChannel.onDidChangeLogLevel?.((logLevel) => {
const newLogLevel = fromVscodeLogLevel(logLevel)
mainLogger.setLogLevel(newLogLevel) // Also logs a message.
})
mainLogger.setLogLevel('debug') // HACK: set to "debug" when debugging the extension.
setLogger(mainLogger)

I see the comment so I am wondering if there something special about the fact that I am running locally? Otherwise this seems like a bug.

Once I manually set the log level (i.e. trigger that event) this doesn't happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when I added that "HACK" line, I assume that something in vscode would always overwrite the log-level, except when running in debug-mode. That line definitely needs some attention.

Perhaps the easiest fix is to wrap that line in if (!isRelease() && ...) or whatever

@Hweinstock Hweinstock marked this pull request as ready for review December 5, 2024 23:17
@Hweinstock Hweinstock requested a review from a team as a code owner December 5, 2024 23:17
@Hweinstock Hweinstock merged commit 3f2ee9e into aws:feature/postreinvent Dec 6, 2024
17 checks passed
@Hweinstock Hweinstock deleted the ec2/loglevel branch December 6, 2024 16:26
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
## Problem
We want to only log file paths when in debug mode, but the connect
script has no way of determining this.

## Solution
- pass log level to connect script. 
- use it to determine what to log. 

Example logs:
``` 
2024/12/05 15:50:24 ==============================================================================
2024/12/05 15:50:24 LOG_LEVEL=3
2024/12/05 15:50:24 AWS_REGION=us-east-1
2024/12/05 15:50:24 SESSION_ID=...
2024/12/05 15:51:11 ==============================================================================
2024/12/05 15:51:11 LOG_LEVEL=1
2024/12/05 15:51:11 AWS_REGION=us-east-1
2024/12/05 15:51:11 SESSION_ID=...
2024/12/05 15:51:11 AWS_SSM_CLI=.../Amazon/sessionmanagerplugin/bin/session-manager-plugin
2024/12/05 15:51:11 LOG_FILE_LOCATION=/.../ec2.{instanceId}.log
2024/12/05 15:51:55 ==============================================================================
2024/12/05 15:51:55 LOG_LEVEL=2
2024/12/05 15:51:55 AWS_REGION=us-east-1
2024/12/05 15:51:55 SESSION_ID=default-uk9cv4h86y5rdbrzarlkj9opci
2024/12/05 15:51:56 AWS_SSM_CLI=../Amazon/sessionmanagerplugin/bin/session-manager-plugin
2024/12/05 15:51:56 LOG_FILE_LOCATION=/.../ec2.{instanceId}.log
```

---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).

License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.
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.

2 participants