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

Add periodic logging messages to Everything server. #847

Merged

Conversation

cliffhall
Copy link
Contributor

@cliffhall cliffhall commented Mar 13, 2025

Description

Send periodic log messages to client, respecting the log level set by the client

  • use setInterval to send a random-leveled log message every 15 seconds
  • import LoggingLevel from sdk
  • add log messages for all levels
  • add isMessageIgnored function that checks the incoming level against the logLevel and returns false if it is a lower index than the logLevel
  • in the setInterval for sending dummy logs, only send the message if it is not ignored by the logLevel.
  • In README.md
    • added example of logging message

Server Details

  • Server: Everything
  • Changes to: Logging

Motivation and Context

In the Inspector, it was reported that log messages from the server were not being displayed. The user who created the issue provided a python test server to send dummy messages, which used FastMCP. I wanted to test this with a reference server that didn't use any outside MCP libraries, to avoid any possible problems in the other project.

I expected that the 'everything server' would be exercising this capability, but it wasn't. So I decided to add that functionality at the same time as fixing the inspector.

How Has This Been Tested?

Used the output from this change to test the Inspector fix (where messages weren't getting through) and that the messages being sent are higher in severity than or equal to the selected level in the Inspector Logging Level dropdown
level-respected-messages

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 Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

modelcontextprotocol/inspector#184

The user who created the issue provided a python test server to send dummy messages, but I expected that the 'everything server' would be exercising this capability, but it wasn't. So I decided to add that functionality at the same time as fixing the inspector.

In src/everything/everything.ts
 - add logsUpdateInterval
 - use setInterval to send a random-leveled log message every 15 seconds
@olaservo
Copy link
Contributor

I haven't worked with as many PRs in this repo yet so I'm not clear on what is causing the failures here - could you update your branch using the latest from main and we can re-run the workflows after that?

@cliffhall
Copy link
Contributor Author

cliffhall commented Mar 15, 2025

The user who created the issue provided a python test server to send dummy messages, which used FastMCP. I wanted to test this with a reference server that didn't use any outside MCP libraries, to avoid any possible problems in the other project.

@olaservo There's not anything broken here, it's just that the Everything server seemed the right place to send some messages to the client for testing that functionality. The problem was really there.

@cliffhall
Copy link
Contributor Author

@olaservo I've got a fix in the UI for letting you send set level requests
Screenshot 2025-03-15 at 2 50 04 PM

The respecting of log level setting by the Everything server depends on the work in this PR. I've started that on another branch. It would be nice if this branch was merged so I can carry on with that. I could alternatively merge this branch into that one and carry on.

@cliffhall cliffhall requested a review from olaservo March 15, 2025 19:11
olaservo
olaservo previously approved these changes Mar 15, 2025
@olaservo
Copy link
Contributor

@cliffhall thanks, sorry by 'failures' I meant the failed checks showing up on the PR here. Looking at other PRs I think this is expected and it allows me to merge, but I'm double checking with other contributors to see if there's any special guidance here.

@cliffhall
Copy link
Contributor Author

cliffhall commented Mar 15, 2025

@cliffhall thanks, sorry by 'failures' I meant the failed checks showing up on the PR here. Looking at other PRs I think this is expected and it allows me to merge, but I'm double checking with other contributors to see if there's any special guidance here.

No idea what the problem with that would be. Looking into it as well.

UPDATE:
Looks like this last merged PR may have introduced something. Seems innocuous though, just wording in markdown links. Maybe there is some CI outage or misconfig right now?
https://github.com/modelcontextprotocol/servers/pull/869/checks
Screenshot 2025-03-15 at 3 43 10 PM

* This fixes modelcontextprotocol#868
* In everything.ts
  - import LoggingLevel from sdk
  - add log messages for all levels
  - add isMessageIgnored function that checks the incoming level against the logLevel and returns false if it is a lower index than the logLevel
  - in the setInterval for sending dummy logs, only send the message if it is not ignored by the logLevel.
@cliffhall
Copy link
Contributor Author

@olaservo I went ahead and added the respect log level functionality to this branch.

@olaservo
Copy link
Contributor

The package-lock was just out of sync so if you get the latest from main again these should all pass now, thank you for your patience.

@cliffhall
Copy link
Contributor Author

Updated the branch and the README.md. Should be all good.

@olaservo olaservo merged commit 8997e96 into modelcontextprotocol:main Mar 17, 2025
25 checks passed
@cliffhall cliffhall deleted the add-logging-to-everything-server branch March 17, 2025 15:10
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.

Respect log level setting by client
2 participants