feat: add shared observability abstractions and wire to runtime log handler - #2169
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2169 +/- ##
============================================
- Coverage 97.16% 97.12% -0.05%
============================================
Files 519 525 +6
Lines 35506 35698 +192
============================================
+ Hits 34500 34672 +172
- Misses 1006 1026 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Scanned the new src/core/observability/* and src/handlers/observability/* modules and their tests. The abstractions look clean and I didn't find any serious issues that need to change before merging:
- The
ObservabilityClient→SourceReader→ resolver split is well-scoped, and the barrel import path (core/observability/index) is intentionally used until the legacysrc/core/observability.tsis migrated (per the in-file comments). - Mocking is kept at proper boundaries —
SourceReaderandCloudWatchLogsClientfakes in unit tests, and the fixture test drives the whole handler stack through recorded SDK responses viafixtureFactories. No excessive mocking. CloudWatchSourceReader.searchLogscorrectly clamps per-pagelimitto 10 000, guards against nextToken cycles, and translatesResourceNotFoundExceptioninto a customer-facingResourceNotFoundErrorfor all three operations.- Live Tail loop handles both the streamed
SessionTimeoutExceptionevent and the thrown-exception form, respectsAbortSignalinside the inner loop, and strips the legacy:*ARN suffix thatStartLiveTailrejects. resolveTimeWindowpinsnow()to a single reference so--since 1hand--until 30mcompute against the same instant, and validates window ordering.- Telemetry is emitted centrally via the router's
cli.command_runwrapper, so no per-handler instrumentation is required here.
Minor nits I intentionally didn't flag (not blocking):
runInsightsQueryre-fetches page 0 after polling completes rather than reusing the results from the final poll response — extra API call, but harmless.sanitizeQueryValueis exported but not yet used inside this PR; presumably picked up by a follow-up caller.
Ship it.
b4ec285 to
956414f
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| options: CoreOptions, | ||
| signal?: AbortSignal, | ||
| ): AsyncGenerator<CloudWatchLogEvent, void> { | ||
| if (query.limit !== undefined && query.limit <= 0) return; |
There was a problem hiding this comment.
Is there a reason why we aren't throwing an InputValidationError? I think we should rather than silently returning.
| let nextToken: string | undefined; | ||
| let yielded = 0; | ||
|
|
||
| do { |
There was a problem hiding this comment.
OOS: We should use create a paginator helper in the future.
Description
This PR adds reusable observability infrastructure and wires the imperative
agentcore runtime logscommand to it. Remaining wiring, as well as traces and project based observability are planned as follow ups to this PR.Each primitive handler resolves its own log source, while the shared observability client handles provider-independent log operations. CloudWatch-specific mechanics remain isolated behind a dedicated client.
Original PR and discussion: PR #2147
ObservabilityClientas the shared API entry pointCloudWatchClientresponsible for CloudWatch request handling and event normalizationagentcore runtime logscommand to the shared infrastructureNote: legacy
observability.tsclient still exists to support project resolution methods and trace methods. As those are OOS for this PR. Follow ups to this PR will contain logic scaffolding project based & trace infra, at which point legacy client will be removed and logic shifted.Architecture
Type of Change
Testing
How have you tested the change?
bun run test(2764 pass, 0 fail)npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.
Stack created with GitHub Stacks CLI • Give Feedback 💬