Add logging channel and more logging to chat agent handler #19398
Conversation
lewis-sanchez
requested review from
kburtram,
aasimkhan30,
cssuh,
caohai,
laurenastrid1 and
Benjin
as code owners
PR Changes
|
| let conversationUri = getNextConversationUri(); | ||
| let connectionUri = vscodeWrapper.activeTextEditorUri; | ||
|
|
||
| Utils.logDebug( | ||
| logger.info("In handler"); |
There was a problem hiding this comment.
This "entered/exited XYZ function" can probably be verbose- instead of info-level logging. Same for the other spots
| @@ -46,17 +47,24 @@ export const createSqlAgentRequestHandler = ( | |||
| return () => `conversationUri${idCounter++}`; | |||
| })(); | |||
|
|
|||
| const getLogger = (() => { | |||
There was a problem hiding this comment.
Is it best practice to create a different logging channel per feature area? This seems like total over kill. I'd expect the extension to have 1 or 2, or maybe 3. But it seems like we're creating these for more smaller feature areas in recent PRs. Is there some guidance from the vscode team this is a good pattern to follow? I prefer to have my logs consolidated generally to be able to spot errors that cross subsystem boundaries (up to a point, or course)
There was a problem hiding this comment.
@Benjin, do you have additional info on why it was suggested to use a MssqlCopilot logger in addition to copilot specific messages being easier to filter?
There was a problem hiding this comment.
This call reuses the same "MSSQL" output channel as the rest of the extension; the second parameter is a string tag that gets included in each message so that you can optionally filter your messages further by searching for [tag] in the MSSQL channel:
Should be easier to both view the full picture and isolate specific areas in the logs.
There was a problem hiding this comment.
Thanks! Prefix label on message in the main MSSQL channel makes sense to me.
|
My take on this PR is that we should probably send most the logging to a single channel, perhaps for very specific logging we might want to isolated it, but I don't think we should create a different logging channel for each feature area. Unless VS Code is recommending extensions do that. |
|
Unless I'm misunderstanding, I think we're already doing that; everything's going to the |
This PR fixes #19363