Skip to content

Conversation

@eray-felek-sonarsource
Copy link
Contributor

No description provided.

@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Feb 10, 2026

MCP-272

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch 5 times, most recently from 6652bf1 to 8090f81 Compare February 11, 2026 11:07
@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch 4 times, most recently from c4df9b6 to 2c7630d Compare February 11, 2026 13:23
@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch from 2c7630d to 498ab1c Compare February 11, 2026 13:33
@eray-felek-sonarsource eray-felek-sonarsource marked this pull request as ready for review February 11, 2026 14:08
@nquinquenel nquinquenel requested a review from Copilot February 11, 2026 14:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces elevated debug logging controlled by the SONARQUBE_ELEVATED_DEBUG environment variable or system property. This feature helps diagnose HTTP communication and tool execution issues by outputting detailed logs to stderr when enabled.

Changes:

  • Added McpLogger.debugElevated() and isElevatedDebugEnabled() methods that check for the SONARQUBE_ELEVATED_DEBUG flag
  • Implemented LoggingHttpClient decorator to log HTTP request/response details (URL, timing, status, body)
  • Enhanced error handling in ServerApiHelper and ToolExecutor to log additional diagnostic information
  • Added configuration logging in HttpClientProvider and SonarQubeMcpServer to output connection settings at startup

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/main/java/org/sonarsource/sonarqube/mcp/log/McpLogger.java Adds elevated debug logging support with environment/property check
src/main/java/org/sonarsource/sonarqube/mcp/http/LoggingHttpClient.java New decorator that logs HTTP operations when elevated debug is enabled
src/main/java/org/sonarsource/sonarqube/mcp/http/HttpClientProvider.java Wraps HTTP clients with logging decorator and exposes connection settings logging
src/main/java/org/sonarsource/sonarqube/mcp/serverapi/ServerApiHelper.java Logs HTTP error details and refactors response body parsing
src/main/java/org/sonarsource/sonarqube/mcp/tools/ToolExecutor.java Logs tool execution start, end, and error details
src/main/java/org/sonarsource/sonarqube/mcp/SonarQubeMcpServer.java Logs sanitized configuration at startup
src/test/java/org/sonarsource/sonarqube/mcp/log/McpLoggerTest.java Comprehensive test coverage for McpLogger functionality
src/test/java/org/sonarsource/sonarqube/mcp/http/LoggingHttpClientTest.java Test coverage for LoggingHttpClient decorator
src/test/java/org/sonarsource/sonarqube/mcp/http/HttpClientProxyTests.java Adds test for proxy settings logging
src/test/java/org/sonarsource/sonarqube/mcp/serverapi/ServerApiTests.java Adds test for error response body logging
src/test/java/org/sonarsource/sonarqube/mcp/SonarQubeMcpServerGenericTest.java Adds test for configuration logging

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@nquinquenel nquinquenel left a comment

Choose a reason for hiding this comment

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

I think the logic is too complex. What I was expecting from this ticket is something way simpler:

  • New SONARQUBE_DEBUG_ENABLED env variable (true/false) in McpServerLaunchConfiguration
  • Documentation update in README
  • New debug() method in McpLogger
  • Simple LOG.debug on various locations we'd like to debug

I don't think we should add this new LoggingHttpClient class

}

private void logSanitizedConfig() {
LOG.debugElevated("=== Sanitized Configuration ===");
Copy link
Member

Choose a reason for hiding this comment

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

What is sanitized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Token is not included

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can come up with a better name

Copy link
Member

Choose a reason for hiding this comment

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

I think we should change this method's name, maybe logDebugDetails ?

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch from a04dc78 to caf5635 Compare February 11, 2026 15:56
@nquinquenel nquinquenel requested a review from Copilot February 12, 2026 09:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@nquinquenel nquinquenel left a comment

Choose a reason for hiding this comment

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

Few comments. I think we could log more meaningful things, related to tools for example. Maybe in a second time...

@sonarqubecloud
Copy link

SonarQube reviewer guide

Review in SonarQube

Summary: Add configurable debug logging with SONARQUBE_DEBUG_ENABLED environment variable to improve troubleshooting, including sanitized configuration details and HTTP error response bodies.

Review Focus:

  • The debug flag implementation in McpLogger.java - ensure the environment variable resolution is thread-safe and handles all edge cases correctly
  • HttpClientProvider refactoring to store SSL/TLS and proxy details for logging - verify no state management issues or memory leaks
  • Response body logging in ServerApiHelper.handleError() - confirm truncation logic (1000 char limit) is appropriate and doesn't expose sensitive data
  • Tool execution logging change from DEBUG to INFO level in McpClientManager - verify this is intentional and won't cause log spam

Start review at: src/main/java/org/sonarsource/sonarqube/mcp/log/McpLogger.java. This is the central control point for the new debug feature and affects behavior across the entire codebase. Ensure the debug flag resolution is robust and the conditional debug output to STDERR is correct.

💬 Please send your feedback

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues
0 Dependency risks

Measures
0 Security Hotspots
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch 2 times, most recently from 436238c to 00cdb85 Compare February 12, 2026 13:00
@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/MCP-272-introduce-elevated-logging branch from 00cdb85 to e18344c Compare February 12, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants