feat(apm): agent file-read enrichment#2836
Open
jonmcwest wants to merge 1 commit into
Open
Conversation
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2 tasks
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/agent/src/enrichment/file-enricher.ts:97-102
**`formatApmInlineComments` called without any error guard**
Every other failure surface in this function — `enrichEventsForAgent`, `getApmLineStats` — returns `null` on error. `formatApmInlineComments` processes external API data (`apmStats`) but is called bare, so an unexpected throw (e.g. a line number out of range, a null stat field, or an edge-case in the formatter) would propagate to the caller as an unhandled rejection instead of silently degrading. Wrapping it in a `try/catch` that falls back to `eventAnnotated ?? content` would keep the defensive pattern consistent.
### Issue 2 of 3
packages/agent/src/enrichment/file-enricher.ts:85-88
**`getApiKey()` called twice in parallel for dual-enrichable files**
Both `enrichEventsForAgent` (via `resolveEnricherConfig(deps, 5_000)`) and `getApmLineStats` (via `resolveEnricherConfig(deps, APM_QUERY_TIMEOUT_MS)`) concurrently call `deps.apiConfig.getApiKey()` for any TypeScript/JS file that contains posthog calls and is also APM-traced — a common scenario in PostHog's own front-end code. If `getApiKey` triggers a token refresh, two parallel refreshes race each other. The reason the timeouts differ (5 s vs 15 s) is valid, but the key itself is invariant to the timeout; resolving it once and passing it into both sub-paths would eliminate the redundant call without changing semantics.
### Issue 3 of 3
packages/agent/src/enrichment/file-enricher.test.ts:316-368
**Cache-behaviour tests could be consolidated into a parameterised suite**
Three consecutive tests — "serves stats from cache (one fetch)", "caches an empty result…", and "a transient fetch failure is not cached" — share the exact same fixture shape (call `enrichFileForAgent` twice with the same path, assert `fetchApmSpy` call count). The team preference is for parameterised tests; a `test.each` table over `[fetchImpl, expectedCallCount, expectedFirstResult]` would express the three variants with less duplication while keeping all assertions intact.
Reviews (1): Last reviewed commit: "feat(apm): agent file-read enrichment" | Re-trigger Greptile |
2 tasks
696d183 to
d02ea61
Compare
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Problem
APM (Application Performance Monitoring) line-level stats were never reaching the host log sink because
ClaudeAcpAgentalways created its own isolatedLoggerinstance. Additionally, file enrichment had no APM annotation capability — production span statistics were not being surfaced as inline comments when the agent read source files.Changes
ClaudeAcpAgentnow accepts an optionalloggerin its options and uses it in place of the internally constructed one, so enrichment logs flow through the host's log sink (e.g.main.log). Theacp-connectionadapter passes a child logger scoped to"ClaudeAcpAgent".enrichFileForAgentnow runs event annotation and APM stat fetching concurrently viaPromise.all. When APM stats are available for a supported language,formatApmInlineCommentsannotates the file content with per-line span data before returning.getApmLineStatshelper manages a path-keyed in-memory cache with a 5-minute TTL. Cache misses trigger afetchApmLineStatscall with a 15-second timeout (sized for the worst-case 24h query window). Transient fetch failures are not cached, allowing retries on the next read. Empty results are cached to avoid redundant fetches for untraced files.resolveEnricherConfigis extracted as a shared helper used by both the event enrichment path (5s timeout) and the APM path (15s timeout).buildWrapperContextare now caught per-import so a single failing resolution cannot abort the entire file read.createEnrichmentwires up theapmStatsCacheand a concretefetchApmLineStatsimplementation backed byPostHogApi.getApmLineStats, and clears the cache ondispose.How did you test this?
claude-agent.logger.test.tsverifying that a host-provided logger receives enrichment log events with the correct prefix and payload.file-enricher.test.tswith an"APM enrichment caching"suite covering: cache hit on repeated reads, empty-result caching, stat re-application after an edit, no caching on transient failure, graceful degradation whengetApiKeyrejects, timeout value assertion, TTL expiry triggering a refetch, and resilience when the event-annotation branch throws.Automatic notifications