-
Notifications
You must be signed in to change notification settings - Fork 333
Cherry-pick: LogLevel bug fixes #3524
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
Open
RubenCerna2079
wants to merge
5
commits into
release/2.0
Choose a base branch
from
dev/rubencerna/cherry-pick-fix-loglevel-bugs-3
base: release/2.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
70d79ce
Fix logs still appearing even when LogLevel is set to `none` bug (#3318)
RubenCerna2079 ee75c86
Add MCP notifications/message for log streaming to clients (#3484)
anushakolan ec08d2a
Merge branch 'release/2.0' into dev/rubencerna/cherry-pick-fix-loglev…
Aniruddh25 daab805
Fix merge conflicts
RubenCerna2079 de85dd1
Fix merge conflict in test
RubenCerna2079 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Text; | ||
|
|
||
| namespace Azure.DataApiBuilder.Mcp.Core | ||
| { | ||
| /// <summary> | ||
| /// Process-wide owner of the MCP stdio server's stdout stream. | ||
| /// | ||
| /// In MCP stdio mode, stdout is the JSON-RPC channel and is shared by | ||
| /// multiple writers — JSON-RPC responses from <see cref="McpStdioServer"/> | ||
| /// and asynchronous <c>notifications/message</c> frames from the logging | ||
| /// pipeline. Without coordination, two writers calling <c>WriteLine</c> | ||
| /// concurrently can interleave at the byte level and corrupt the channel. | ||
| /// | ||
| /// This class wraps the underlying <see cref="StreamWriter"/> and serializes | ||
| /// every write through a single lock so JSON-RPC frames stay intact. | ||
| /// Registered as a singleton in DI for MCP stdio mode; instantiated lazily | ||
| /// (the underlying stream is opened on the first write) so non-MCP code | ||
| /// paths and unit tests can construct the type without side effects. | ||
| /// </summary> | ||
| public sealed class McpStdoutWriter : IDisposable | ||
| { | ||
| private readonly object _lock = new(); | ||
| private TextWriter? _writer; | ||
| private bool _disposed; | ||
|
|
||
| /// <summary> | ||
| /// Production constructor. The underlying stdout stream is opened | ||
| /// lazily on the first <see cref="WriteLine"/> call. | ||
| /// </summary> | ||
| public McpStdoutWriter() | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Test-only constructor that injects a pre-built writer so unit tests | ||
| /// can verify lock behavior, disposal semantics, and notification | ||
| /// framing without touching the real stdout stream. | ||
| /// </summary> | ||
| internal McpStdoutWriter(TextWriter writer) | ||
| { | ||
| _writer = writer ?? throw new ArgumentNullException(nameof(writer)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Writes a single line to stdout under a process-wide lock so | ||
| /// concurrent JSON-RPC responses and notifications cannot interleave. | ||
| /// No-op after <see cref="Dispose"/>. | ||
| /// </summary> | ||
| public void WriteLine(string line) | ||
| { | ||
| lock (_lock) | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| EnsureInitialized(); | ||
| _writer!.WriteLine(line); | ||
| } | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| lock (_lock) | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _disposed = true; | ||
| _writer?.Dispose(); | ||
| _writer = null; | ||
| } | ||
| } | ||
|
|
||
| private void EnsureInitialized() | ||
| { | ||
| if (_writer is not null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Opening the raw stdout stream bypasses any Console.SetOut(...) | ||
| // redirection. This is intentional: in MCP stdio mode, Program.cs | ||
| // redirects Console.Out to a sink (TextWriter.Null or stderr) so | ||
| // stray Console.WriteLine calls from third-party code cannot | ||
| // corrupt the JSON-RPC channel. Only this class - and only via | ||
| // WriteLine() - is allowed to write to the real stdout. | ||
| Stream stdout = Console.OpenStandardOutput(); | ||
| _writer = new StreamWriter(stdout, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)) | ||
| { | ||
| AutoFlush = true | ||
| }; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
114 changes: 114 additions & 0 deletions
114
src/Azure.DataApiBuilder.Mcp/Telemetry/McpLogNotificationWriter.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Text.Json; | ||
| using Azure.DataApiBuilder.Core.Telemetry; | ||
| using Azure.DataApiBuilder.Mcp.Core; | ||
| using Azure.DataApiBuilder.Mcp.Model; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace Azure.DataApiBuilder.Mcp.Telemetry; | ||
|
|
||
| /// <summary> | ||
| /// Writes log messages as MCP `notifications/message` JSON-RPC notifications. | ||
| /// This allows MCP clients (like MCP Inspector) to receive log output in real-time. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// MCP spec: https://modelcontextprotocol.io/specification/2025-11-05/server/utilities/logging | ||
| /// The notification format is: | ||
| /// <code> | ||
| /// { | ||
| /// "jsonrpc": "2.0", | ||
| /// "method": "notifications/message", | ||
| /// "params": { | ||
| /// "level": "info", | ||
| /// "logger": "CategoryName", | ||
| /// "data": "The log message" | ||
| /// } | ||
| /// } | ||
| /// </code> | ||
| /// All writes are routed through the shared <see cref="McpStdoutWriter"/> so | ||
| /// notifications cannot interleave with JSON-RPC responses written by | ||
| /// <see cref="McpStdioServer"/>. | ||
| /// </remarks> | ||
| public class McpLogNotificationWriter : IMcpLogNotificationWriter | ||
| { | ||
| private readonly McpStdoutWriter? _stdoutWriter; | ||
|
|
||
| /// <summary> | ||
| /// Creates a notification writer that writes through the shared stdout | ||
| /// writer. The shared writer serializes notifications with JSON-RPC | ||
| /// responses so concurrent writes do not interleave on the wire. | ||
| /// </summary> | ||
| /// <param name="stdoutWriter"> | ||
| /// Shared stdout writer. May be <c>null</c> for unit tests that do not | ||
| /// exercise the write path; in that case <see cref="WriteNotification"/> | ||
| /// becomes a no-op. | ||
| /// </param> | ||
| public McpLogNotificationWriter(McpStdoutWriter? stdoutWriter = null) | ||
| { | ||
| _stdoutWriter = stdoutWriter; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets whether MCP log notifications are enabled. This is the | ||
| /// single source of truth for whether notifications flow to the client; | ||
| /// it is consulted by <see cref="McpLogger.IsEnabled(LogLevel)"/> so that | ||
| /// the gate is enforced once, at log time, before any formatter work runs. | ||
| /// <see cref="WriteNotification"/> intentionally does not re-check this | ||
| /// flag — callers must gate via <see cref="McpLogger"/>. | ||
| /// </summary> | ||
| public bool IsEnabled { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Writes a log message as an MCP notification. The caller is responsible | ||
| /// for gating on <see cref="IsEnabled"/>; <see cref="McpLogger"/> already | ||
| /// does this in its <see cref="McpLogger.IsEnabled(LogLevel)"/> override. | ||
| /// </summary> | ||
| /// <param name="logLevel">The .NET log level.</param> | ||
| /// <param name="categoryName">The logger category (typically class name).</param> | ||
| /// <param name="message">The formatted log message.</param> | ||
| public void WriteNotification(LogLevel logLevel, string categoryName, string message) | ||
| { | ||
| // No IsEnabled check here: the gate lives in McpLogger.IsEnabled so | ||
| // that we have a single source of truth and a single check site. | ||
| // The _stdoutWriter null check remains as a defensive guard for unit | ||
| // tests that construct the writer without a backing stdout. | ||
| if (_stdoutWriter is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string mcpLevel = McpLogLevelConverter.ConvertToMcp(logLevel); | ||
|
|
||
| var notification = new | ||
| { | ||
| jsonrpc = McpStdioJsonRpcErrorCodes.JSON_RPC_VERSION, | ||
| method = "notifications/message", | ||
| @params = new | ||
| { | ||
| level = mcpLevel, | ||
| logger = categoryName, | ||
| data = message | ||
| } | ||
| }; | ||
|
|
||
| _stdoutWriter.WriteLine(JsonSerializer.Serialize(notification)); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Interface for MCP log notification writing. | ||
| /// </summary> | ||
| public interface IMcpLogNotificationWriter | ||
| { | ||
| /// <summary> | ||
| /// Gets or sets whether MCP log notifications are enabled. | ||
| /// </summary> | ||
| bool IsEnabled { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Writes a log message as an MCP notification. | ||
| /// </summary> | ||
| void WriteNotification(LogLevel logLevel, string categoryName, string message); | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.