Skip to content

Migrate official MCP SDK logging from Logrus to slog - #505

Open
glbyktjys wants to merge 2 commits into
mainfrom
refactor-logger-to-slog
Open

Migrate official MCP SDK logging from Logrus to slog#505
glbyktjys wants to merge 2 commits into
mainfrom
refactor-logger-to-slog

Conversation

@glbyktjys

Copy link
Copy Markdown
Contributor

This PR:

  • Initializes a dedicated slog logger when the official MCP server starts in HTTP mode.
  • Passes the logger to the official SDK for server and session logging.
  • Updates the tool-logging middleware to record successful calls, protocol errors, and tool execution errors using structured slog records.
  • Writes official SDK logs to terraform-mcp-official.log, keeping them isolated from the existing Logrus output.

What hasn’t changed:

  • Stdio logging remains on Logrus.
  • Shared HTTP wrappers continue using Logrus to preserve the existing server behavior.
  • Official client initialization remains on Logrus and will be migrated after the HTTP client and session-management changes are complete.

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

"tool", params.Name, "arguments", string(params.Arguments))
toolName := "unknown"
arguments := ""
if params, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the defensive check on logger != nil was removed here. Do we not need it anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that! I missed preserving the nil guard while expanding the logging logic. I’ll restore it so the middleware continues without logging if the logger is nil.

Comment thread cmd/terraform-mcp-server/init.go Outdated
Version string `json:"version"`
}

const officialSlogOutputPath = "terraform-mcp-official.log"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this const be renamed to officialSlogOutputFileName instead of Path? I'm just wondering where it tries to look for this file in initSlog. Is it in the cwd?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the new name would be clearer. It’s a relative filename, so the log is created in the cwd. If the server restarts from the same directory, it appends to the existing file. I’ll rename it accordingly.

@glbyktjys
glbyktjys force-pushed the refactor-logger-to-slog branch from a1fa929 to 40157a6 Compare September 4, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants