feat: add traces to observability client and wire to runtime handlers - #2180
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2180 +/- ##
============================================
- Coverage 97.15% 97.08% -0.07%
============================================
Files 542 544 +2
Lines 37740 37768 +28
============================================
+ Hits 36665 36666 +1
- Misses 1075 1102 +27 ☔ 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
Nice consolidation of trace logic into the shared observability layer, mirroring the earlier runtime logs refactor. Tests use real temp dirs (good), and telemetry is handled at the router layer so no per-handler instrumentation is needed.
A couple of behavior changes to runtime traces that are worth an explicit callout in the PR description (or a confirmation that they’re intentional) — they’re not necessarily wrong, but they aren’t mentioned in the PR body:
-
--idis now required for bothruntime traces listandruntime traces get. Previously it wasruntimeIdSchema.optional()with a "defaults to the project's deployed runtime" fallback viaresolveRuntimeTarget. The newruntimeFlagsinsrc/handlers/runtime/traces/{list,get}/index.tsxdrops that path. This matches whatruntime logsdid in #2169, so probably intentional — just want to confirm. -
getdefault output path changed fromagentcore/.cli/traces/<runtimeId>-<traceId>.json(when inside a project) to<traceId>.jsonin the cwd, always. The deletedsrc/handlers/runtime/traces/get/outputPath.tsused to be project-aware and prefixed the filename with the runtime id so per-runtime downloads didn’t collide. The newsrc/handlers/runtime/traces/outputPath.tsand theoutputDescriptioninget/index.tsxunconditionally drop into cwd with just<traceId>.json. If a user in a project pulls the same trace id from two different runtimes back-to-back they’ll now overwrite. If this is deliberate (and not a regression from the shared-abstraction simplification) it’d be worth calling out — the PR body only mentions the atomic write.
Minor, non-blocking:
- In
src/handlers/observability/traces.ts,getTraceFlags(line 52) is defined only so itstypeofcan produceGetTraceFlagValues;createGetTraceHandlerthen reconstructs the same flag list inline at lines 151–155. It works, but the two definitions can drift. Either derive the runtimegetFlagsfromgetTraceFlags([...config.flags, ...getTraceFlags]) or drop the top-level constant and type the values structurally — reviewer’s choice.
Everything else (validation in listTracesInsightsQuery/getTraceInsightsQuery, the TRACE_RECORD_LIMIT warning path, atomicWrite for the JSON dump, and the TestObservabilityClient shape) looks solid. Happy to see this merge once the behavior changes above are acknowledged.
3b82240 to
6c96b59
Compare
|
Claude Security Review: no high-confidence findings. (run) |
6c96b59 to
06704a0
Compare
|
Claude Security Review: no high-confidence findings. (run) |
06704a0 to
6e743cd
Compare
|
Claude Security Review: no high-confidence findings. (run) |
6e743cd to
ae85669
Compare
|
Claude Security Review: no high-confidence findings. (run) |
jariy17
left a comment
There was a problem hiding this comment.
nit: SessionIds are custom attributes for traces. So don't expect every trace to have a sessionId attribute. This should be fine for agents' traces but I'm not sure for gateway and etc.
ae85669 to
1a341a5
Compare
|
Claude Security Review: no high-confidence findings. (run) |
1a341a5 to
4fa3550
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Description
Add trace logic in shared observability abstraction established for logging, and wire runtime handlers to use it.
listTracesandgetTraceoperationsTRACE_RECORD_LIMITNote:
--idflag has become required when using observability commands. This is intentional. Previous logic mixed project resolution with imperative command, which is why id was supported as optional.Type of Change
Testing
How have you tested the change?
bun run test(2788 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.