Skip to content

Multi transport logging #42

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Mar 10, 2025
Merged

Multi transport logging #42

merged 11 commits into from
Mar 10, 2025

Conversation

MattMorgis
Copy link
Contributor

Allows for a config like this:

logger:
  transports: [file, console]
  level: debug
  path_pattern: "logs/mcp-agent-{timestamp}.jsonl"
  timestamp_format: "%Y%m%d_%H%M%S"

Keeps backward compatibility:

logger:
  type: file
  level: debug
  path: "logs/mcp-agent.jsonl"

@MattMorgis MattMorgis mentioned this pull request Mar 7, 2025
Open
saqadri
saqadri requested changes Mar 8, 2025
View reviewed changes
Copy link
Collaborator

@saqadri saqadri left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @MattMorgis! Directionally looks good to me, but I have a few suggestions which would be great to incorporate before merging. Please let me know if you have any questions on them, and thanks again!

saqadri
saqadri reviewed Mar 8, 2025
View reviewed changes
@MattMorgis
Copy link
Contributor Author

@saqadri I think I addressed all of your feedback. I also updated the README and the basic agent example to reflect the new config options. Let me know if you need anything else!

saqadri
saqadri approved these changes Mar 10, 2025
View reviewed changes
Copy link
Collaborator

@saqadri saqadri left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the feedback. Really nice work @MattMorgis!

src/mcp_agent/config.py
@@ -183,6 +213,12 @@ class LoggerSettings(BaseModel):
path: str = "mcp-agent.jsonl"
"""Path to log file, if logger 'type' is 'file'."""

# Settings for advanced log path configuration
path_settings: Optional["LogPathSettings"] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, any reason for referencing the type as "LogPathSettings" instead of LogPathSettings? Could do LogPathSettings | None if you drop the quotations. Just curious, not a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, nice catch. A combination of Claude doing it + me still new enough at Python to not notice. I can submit a follow up, the rest of the file uses the | None pattern

src/mcp_agent/logging/transport.py
Comment on lines +503 to +504
# Default fallback
return settings.path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps at this point we should raise ValueError because we should have been able to generate something valid by this point?

@saqadri saqadri merged commit 8d2da8f into lastmile-ai:main Mar 10, 2025
4 checks passed
@MattMorgis MattMorgis deleted the update-logging branch March 10, 2025 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saqadri saqadri saqadri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MattMorgis @saqadri