-
Notifications
You must be signed in to change notification settings - Fork 14
Implement unified logger with log level -- part 1 #93
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
base: main
Are you sure you want to change the base?
Conversation
…fied-logger-for-MCP
MacondoExpress
left a comment
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.
Really good! Left some comments, nothing of these are massive blocker IMO
| } | ||
| } | ||
|
|
||
| func replaceAttr(_ []string, a slog.Attr) slog.Attr { |
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.
From the reader perspective is not clear the intention of why we need the replaceAttr func rather than relying on the slog formatter.
From what I guess, it's because slog convert levels to string using a small subset of values:
- DEBUG
- INFO
- WARN
- ERROR
Giving back less granularity than what we desire, but as it's not tested the underline reason could be lost over time!
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 added a doc string to explain what replaceAttr is used for. We also have a test already all log levels can be set dynamically and now we are checking for valid values so hopefully it should be a lot more clear.
Introduce a logging service to enhance error handling and debugging capabilities across various components of the application. Update configuration to include log level and format, and ensure that logging is integrated into the server and tool dependencies.
WIP: only integrated for the get-schema tool for now. Tests to follow.
Part 1: (in this PR)
logger only implemented for the get-schema tool for nowimplemented for all toolsPart 2: (not in this PR but here #95)