Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the STS (SQL Tools Service) initialization logging to better capture startup behavior, helping diagnose initialization failures like the one reported in issue #20635. The code is also refactored from nested Promise chains (then/catch/reject) to modern async/await syntax, improving readability and maintainability.
Changes:
- Refactored
initializeandinitializeForPlatforminserviceclient.tsfrom Promise-chain style toasync/await, and added a post-initialization log message summarizing the result. - Changed
ServerInitializationResultinserverStatus.tsto use primitivebooleantypes instead of the boxedBooleanwrapper type. - Improved error logging in the catch block to use
getErrorMessage(err)for better error message extraction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
extensions/mssql/src/languageservice/serviceclient.ts |
Refactored initialize and initializeForPlatform to async/await, added post-init logging, and improved error message formatting. |
extensions/mssql/src/languageservice/serverStatus.ts |
Changed Boolean (boxed) to boolean (primitive) for installedBeforeInitializing, isRunning, and the withRunning parameter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
PR Changes
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (16.21%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #21482 +/- ##
==========================================
- Coverage 73.24% 73.24% -0.01%
==========================================
Files 328 328
Lines 99403 99399 -4
Branches 5644 5643 -1
==========================================
- Hits 72808 72803 -5
- Misses 26595 26596 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| "assert": "^1.4.1", | ||
| "azdataGraph": "github:Microsoft/azdataGraph#0.0.137", | ||
| "chai": "^5.3.3", | ||
| "chai-as-promised": "^8.0.2", |
There was a problem hiding this comment.
Adding this package is the only change here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| public serverPath: string = undefined, | ||
| public message: string = undefined, |
There was a problem hiding this comment.
ServerInitializationResult can carry serverPath/message as undefined (e.g., failures before a path is resolved), but the constructor types them as string. This makes the API misleading and encourages callers to treat them as always present. Consider typing these as string | undefined (or optional) to match actual values being passed around.
| public serverPath: string = undefined, | |
| public message: string = undefined, | |
| public serverPath: string | undefined = undefined, | |
| public message: string | undefined = undefined, |
| serverPath, | ||
| ); | ||
| } catch (err) { | ||
| this.logger.logDebug(Constants.serviceLoadingFailed + ": " + getErrorMessage(err)); |
There was a problem hiding this comment.
In the catch block, getErrorMessage(err) only logs err.message for Error instances, which drops stack traces/context that are critical for diagnosing startup failures like TypeError ... (reading 'call'). Consider also logging err.stack when available (or a richer serialization) so the output channel/debug logs capture where initialization failed.
| this.logger.logDebug(Constants.serviceLoadingFailed + ": " + getErrorMessage(err)); | |
| this.logger.logDebug(Constants.serviceLoadingFailed + ": " + getErrorMessage(err)); | |
| if (err instanceof Error && err.stack) { | |
| this.logger.logDebug(err.stack); | |
| } |
Description
Improving STS initialization logging to capture startup behavior
Fixes #20635
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines