Skip to content
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

chore: reduce noisy logging #83

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pimeys
Copy link

@pimeys pimeys commented Apr 2, 2025

Motivation and Context

For our users, it's not super polite to spam log messages for everything that happens in the MCP server. We should make these to the debug level, including one warning that was about LLM hallucinating more than anything being seriously broken in the MCP server.

The info/warn/error channels should be more quiet, because in a busy server they will hide actually important logs.

How Has This Been Tested?

Tried it out in our server, saw reduced amount of logs.

Breaking Changes

Nope.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

If this is not what you want, let's work together to calm down the logs a bit. It's rough to fork a project this early and then rebase fixes every week...

@pimeys pimeys force-pushed the reduce-logging branch 2 times, most recently from 9a51eef to b3d9724 Compare April 2, 2025 08:08
@4t145
Copy link
Collaborator

4t145 commented Apr 2, 2025

Sounds reasonable. But I think things like conncetion establishment and initialization, those happen once per conncetion, deserve a info level log, while request/notification/result indeed should be downgraded to debug level. And serve loop inner event should have a trace level.

Is it acceptable? After all you can configure rmcp=warn if you don't want those message. (By default, the log level of tracing is warn.)

@jokemanfire What's your opinion on this?

@pimeys
Copy link
Author

pimeys commented Apr 2, 2025

Yeah. We could do that. I just have some data from customers who do 100B requests per month, and in these cases the only logs you care about is if something goes awry in the code and needs engineer attention. So maybe consider that here too, it's great when you play around with it, but deploying it to the big customers and suddenly they start asking why we log so much :)

@jokemanfire
Copy link
Collaborator

jokemanfire commented Apr 2, 2025

Sounds reasonable. But I think things like conncetion establishment and initialization, those happen once per conncetion, deserve a info level log, while request/notification/result indeed should be downgraded to debug level. And serve loop inner event should have a trace level.听起来合理。但我认为像连接建立和初始化这样的操作,每个连接只发生一次,值得记录 info 级别的日志,而请求/通知/结果确实应该降低到 debug 级别。并且服务循环内部事件应该有 trace 级别的日志。

Is it acceptable? After all you can configure rmcp=warn if you don't want those message. (By default, the log level of tracing is warn.)是否可以接受?毕竟,如果你不希望看到这些消息,你可以配置 rmcp=warn 。(默认情况下,跟踪的日志级别为 warn 。)

@jokemanfire What's your opinion on this?您的看法是什么?

As you said, I believe that the logs for initializing the server should not be downgraded, but the logs in the process are suitable for downgrading, just like most services do. We can carefully examine which logs are suitable for downgrading.And we need to separate which logs are worth warning about.

@jokemanfire
Copy link
Collaborator

jokemanfire commented Apr 2, 2025

I think valuable logs should not be missed just to reduce the number of logs. Changing all logs to debug level, although it will reduce the number of logs, especially all debug logs, will make developers ignore many valuable logs.
How we provide a pram like -v --verbose or -d --debug? if verbose will print all , but warn info will not be ignore .

@EItanya
Copy link
Contributor

EItanya commented Apr 2, 2025

I agree with both parties. The issue is that when using this as a library, the consumer needs to be able to control the logging.

@jokemanfire
Copy link
Collaborator

jokemanfire commented Apr 3, 2025

How about this? code like this ,just wrap in a func? This way, both the generation and debugging environments can meet the requirements. And use env like VERBOSE.

let verbose = env::var("VERBOSE").is_ok();
let event = tracing::event!(
    tracing::Level::INFO,
    ?peer_info,
    message = if verbose { Some("Service initialized as client") } else { None }
);

if verbose {
    event.field("message", &"Service initialized as client");
}

event.record();

@EItanya
Copy link
Contributor

EItanya commented Apr 3, 2025

In general adding magic env vars for logging is not a great idea as they can be hard to discover. I agree with all points made here, but I think the ability to set rmcp=warn should be enough for all cases. If you are using the library directly it is nice nice to have the information, but then in prod one can simply tune the noise with rmcp=warn. What do you think @pimeys?

@4t145 4t145 added this to the Pre release milestones milestone Apr 7, 2025
@4t145
Copy link
Collaborator

4t145 commented Apr 7, 2025

I will tune the log level with a new PR in several days

@4t145 4t145 self-assigned this Apr 7, 2025
@rikhuijzer

This comment was marked as outdated.

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.

5 participants