Conversation
| @@ -150,6 +150,11 @@ export class Tool<Args extends ZodRawShape | undefined = undefined> { | |||
| const username = tableauAuthInfo?.username; | |||
|
|
|||
| this.logInvocation({ requestId, args, username }); | |||
There was a problem hiding this comment.
logInvocation is poorly named now since it sends notifications. What feels right? Rename to something like notifyOfInvocation? Or rename to logAndNotify and move the call to log inside it?
| "name": "@tableau/mcp-server", | ||
| "description": "Helping agents see and understand data.", | ||
| "version": "1.18.7", | ||
| "version": "1.20.0", |
There was a problem hiding this comment.
We've historically bumped the minor version when making a breaking change like this but to follow the principles of semver properly, we really should be making major version bumps for any breaking change.
| const message = JSON.stringify(entry); | ||
| if (config.transport === 'http') { | ||
| // eslint-disable-next-line no-console -- console.log is intentional here since the transport is not stdio. | ||
| console.log(message); |
There was a problem hiding this comment.
I think we should extend LogParams to include an optional error?: unknown field, that way we can be smarter about how we log the error instead of just including it as part of the message using a template literal (which will remove important information from the error like stack trace). Thoughts?
if (config.transport === 'http') {
if (error) {
// LOG THE WHOLE ERROR
console.log(message, error);
// OR JUST A SUBSET OF THE ERROR
console.log(message, { message: error.message, stack: error.stack });
} else {
console.log(message);
}
} else {
process.stderr.write(message.endsWith('\n') ? message : `${message} ${error}\n`);
if (error) {
process.stderr.write(${error}\n`);
}
}2 participants
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Pull Request Template
Introduces a simplified three-level logging system (debug | info | error) with environment-variable-driven severity filtering, consolidates the logging API surface, and adds structured log() calls across previously unlogged or inconsistently logged code paths.
Breaking change: DEFAULT_LOG_LEVEL renamed to DEFAULT_NOTIFICATION_LEVEL to distinguish it from the new LOG_LEVEL env var. DEFAULT_NOTIFICATION_LEVEL controls what's sent to the MCP client; LOG_LEVEL controls what's written to console/file.
New env var: LOG_LEVEL controls server-side log output severity (debug, info, error). Defaults to info.
Description
Motivation and Context
Type of Change
How Has This Been Tested?
Related Issues
Checklist
npm run version. For example,use
npm run version:patchfor a patch version bump.environment variable or changing its default value.
Contributor Agreement
By submitting this pull request, I confirm that:
its Contribution Checklist.