-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: Interactive service auditing in batches #1530
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1530 +/- ##
==========================================
+ Coverage 89.46% 89.51% +0.05%
==========================================
Files 724 726 +2
Lines 50966 51082 +116
Branches 8145 8162 +17
==========================================
+ Hits 45596 45727 +131
+ Misses 3459 3446 -13
+ Partials 1911 1909 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a84f673 to
f735675
Compare
1cd7416 to
64923f3
Compare
...tch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/batch_processing_utils.py
Show resolved
Hide resolved
src/cloudwatch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/server.py
Outdated
Show resolved
Hide resolved
...tch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/batch_processing_utils.py
Show resolved
Hide resolved
90618cb to
4ea1df8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| sessions_by_age = sorted(_batch_sessions.items(), key=lambda x: x[1].get('created_at', '')) | ||
|
|
||
| # Remove oldest sessions until we're under the limit | ||
| excess_count = len(_batch_sessions) - MAX_BATCH_SESSIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks the max sess limit is set to 1 by default, does it mean we don't support MCP to work with more than 1 session at the same time?
| excess_count = len(_batch_sessions) - MAX_BATCH_SESSIONS | ||
| for i in range(excess_count): | ||
| session_id, _ = sessions_by_age[i] | ||
| del _batch_sessions[session_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a race condition risk here?
| Returns: | ||
| Session ID for tracking the batch processing | ||
| """ | ||
| session_id = str(uuid.uuid4()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, MCP server should be stateless. And it looks like you're generating random session id as key for the cache for each call. Does cache really work?
Fixes
Summary
Changes
This PR implements interactive batch processing for service auditing in the CloudWatch Application Signals MCP server. The changes introduce a new workflow that automatically processes large service lists in manageable batches, allowing users to interactively decide whether to investigate findings or continue processing.
Key additions:
batch_processing_utils.py) - Core logic for creating, managing, and processing audit batchesbatch_tools.py) - MCP tools for continuing batch processingUser experience
Before this change:
After this change:
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change? (Y/N)
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.