-
Notifications
You must be signed in to change notification settings - Fork 62
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
chore: add language server logs VSCODE-447, VSCODE-448, VSCODE-449 #563
Conversation
); | ||
|
||
if (editor?.document.languageId !== 'Log') { | ||
if (isPlaygroundEditor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be that vscode chnages log names and we set logs as active playground editors.
@@ -38,14 +34,12 @@ export default class ExplorerController { | |||
}; | |||
|
|||
activateConnectionsTreeView(): void { | |||
log.info('Activating explorer controller...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such logs turned out to be distracting and not helpful for debugging, so removing them.
@@ -134,6 +147,10 @@ export default class LanguageServerController { | |||
async evaluate( | |||
playgroundExecuteParameters: PlaygroundEvaluateParams | |||
): Promise<ShellEvaluateResult> { | |||
log.info('Running a playground...', { | |||
connectionId: playgroundExecuteParameters.connectionId, | |||
inputLength: playgroundExecuteParameters.codeToEvaluate.length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to store users' input, but a general understanding of how big their queries can be insightful. For example, intelliSense cannot handle big complex files or cause other problems with the active editor.
log.info('Evaluate response', { | ||
namespace: res?.result?.namespace, | ||
type: res?.result?.type, | ||
outputLength: JSON.stringify(res?.result?.content || '').length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with output. Maybe we receive something big that crashes a rendering of the response virtual editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niceee, I really like the logging improvements. Left a couple questions/suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
.getConfiguration('mdb.connectionSaving') | ||
.get('defaultConnectionSavingLocation'); | ||
|
||
log.info('Activating extension...', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking a test for this log might be a very useful one for debugging the environment. Not needed for this pr if we don't want to put the time in now, just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you test here? Do we have tests for logs already somewhere in compass?
@@ -551,7 +559,7 @@ export default class ConnectionController { | |||
throw connectError; | |||
} | |||
|
|||
log.info('Successfully connected'); | |||
log.info('Successfully connected', { connectionId }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the connection name as well might help debugging as it can be hard to infer which connection the connectionId belongs to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't pass the connection name anywhere outside the connection controller, not sure we can use it to match connectivity issues here and for example in LS. But I added a name to the explorer logs, so we can find the info what was expanded:
https://github.com/mongodb-js/vscode/pull/563/files#diff-06a415e2a54193ec0955688552bf1cb4813ac5868fc9392083f3c37f8f65a8c4R67-R69
🎉 |
Description
Split from VSCODE-431 (Add additional logging for log file) to shed some light on VSCODE-448 (The playground’s active connection does not match the extension’s active connection).
Additionally stub the 'trackNewConnection' telemetry event and skip the telemetry live connection test to fix CI because of the issue with the Insiders 1.81.0.
The new logs have shown, that some unexpected errors may cause the language server to shut down, which restarts automatically but then misses required for its functioning parameters. We will handle those cases when the MongoDBService re-initializes itself and use the previously used extensionPath and connection details from LanguageServerController. We will also inform users that
'An internal error has occurred. The playground services have been restored.'
.The current PR does not solve the cause of the VSCODE-448 problem (the ticket will remain open for further investigation), but it improves the error messaging and prevents the extension from crashing.
Checklist
Motivation and Context
Types of changes