improve performance time on vercel for traces#2070
Conversation
🦋 Changeset detectedLatest commit: 882a7d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(6) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
agents-manage-ui/src/app/api/signoz/route.ts:94-112Batch mode bypasses time range validation - 🟠 Major:
agents-api/src/domains/manage/routes/signoz.ts:230-245Missing 401/403 error handling and logging consistency
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
agents-api/src/domains/manage/routes/signoz.ts:175-180Missing access denied logging - 🟡 Minor:
agents-api/src/domains/manage/routes/signoz.ts:187-189Missing "SigNoz not configured" logging
💭 Consider (2) 💭
💭 1) agents-api/src/domains/manage/routes/signoz.ts:197-227 Add debug logging for request lifecycle
Issue: The /query-batch endpoint lacks the debug logging statements present in the peer /query endpoint (processing request, security filters enforced, proxying to SigNoz, query successful).
Why: Debug logging aids in tracing request flow during development and incident investigation.
Fix: Consider adding logger.debug() calls at key points: before executing step 1, after step 1 success, before step 2, after step 2 success.
💭 2) agents-api/src/domains/manage/routes/signoz.ts No tests for new endpoint
Issue: The new /query-batch endpoint and injectConversationIdFilter function have no unit tests. Neither does the existing enforceSecurityFilters function (which is security-critical).
Why: Without tests, regressions in authorization checks, filter injection, or error handling could go undetected. The enforceSecurityFilters function is particularly important as it enforces tenant isolation.
Fix: Consider adding tests for:
- Missing payload validation (400 responses)
- Project access denial (403 responses)
- System/API key user bypass
- Empty conversation results (
detailResponse: null) - Conversation ID injection into detail query
- SigNoz unavailability (503 responses)
Refs:
- No existing tests found in
agents-api/src/__tests__/manage/for signoz routes
🚫 REQUEST CHANGES
Summary: This PR introduces a valuable performance optimization by reducing browser-to-server round trips for the traces dashboard. The security filters for tenant isolation are correctly applied to both payloads using the existing enforceSecurityFilters function. However, two issues should be addressed before merging:
- Time range validation consistency — the batch mode path should validate time ranges like the single query path does
- Error handling parity — the error handler should match the existing
/queryendpoint's handling of 401/403 errors and include appropriate logging
The minor logging inconsistencies (access denied, SigNoz not configured) would be nice to address for operational parity but are not blocking.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
route.ts |
Different timeout values (30s vs 60s) | Intentional design decision — batch executes 2 sequential queries |
route.ts |
Inconsistent error field naming (message vs details) |
Existing pattern where proxy wraps API errors |
signoz-stats.ts:206-212 |
Retry on 5xx may amplify cascading failures | Valid concern but existing pattern; out of scope for this PR |
signoz-stats.ts:180-187 |
Redundant retry configuration | Existing pattern; may be intentional redundancy |
signoz-stats.ts:244-257 |
Silent failure in getConversationStats | Pre-existing pattern not introduced by this PR |
signoz.ts:204-227 |
Timeout budget exhaustion risk | Theoretical risk; 60s total with 30s per query has margin |
signozHelpers.ts |
No tests for enforceSecurityFilters | Pre-existing gap not introduced by this PR |
signoz-stats.ts:362-387 |
No tests for client-side pagination logic | Client-side code; likely covered by E2E tests |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
6 | 0 | 0 | 0 | 2 | 0 | 4 |
pr-review-sre |
7 | 0 | 0 | 0 | 1 | 0 | 6 |
pr-review-consistency |
7 | 0 | 1 | 0 | 2 | 0 | 4 |
pr-review-standards |
2 | 0 | 0 | 0 | 2 | 0 | 0 |
pr-review-tests |
5 | 0 | 1 | 0 | 0 | 0 | 4 |
pr-review-security-iam |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 28 | 0 | 2 | 0 | 4 | 0 | 18 |
Note: High discard rate due to semantic deduplication (multiple reviewers flagged same issues) and filtering pre-existing patterns.
|
@claude will this improve performance time for loading traces on vercel |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a re-review scoped to the delta since the last automated review. The delta commits address all 4 issues raised in the prior review.
Prior Feedback — Addressed ✅
All 4 inline comments from the previous review have been resolved:
| Prior Issue | Status | Resolution |
|---|---|---|
| 🟠 Batch mode bypasses time range validation | ✅ Addressed | Added validation for both paginationPayload and detailPayloadTemplate (lines 108-134 in route.ts) |
| 🟠 Missing 401/403 error handling | ✅ Addressed | Added auth error handling with logging (lines 244-250 in signoz.ts) |
| 🟡 Missing access denied logging | ✅ Addressed | Added logger.warn() before 403 response (lines 176-179 in signoz.ts) |
| 🟡 Missing "SigNoz not configured" logging | ✅ Addressed | Added logger.error() before 500 response (line 192 in signoz.ts) |
Delta Verification
The delta changes were validated by pr-review-standards and pr-review-consistency:
- ✅ Error handling and validation logic is correct
- ✅ Logging patterns match the existing
/queryendpoint - ✅ Time range validation covers both payloads in batch mode
💭 Consider (from prior review, still applicable)
The prior review noted two "Consider" items that remain valid suggestions for future work, but are not blocking:
-
Debug logging for request lifecycle — The
/query-batchendpoint could benefit from debug logging at key points (before/after step 1, before/after step 2) to match the existing/queryendpoint's observability. -
Tests for new endpoint — The
/query-batchendpoint andinjectConversationIdFilterfunction have no unit tests. Consider adding tests for validation, authorization, and error handling paths.
✅ APPROVE
Summary: All feedback from the prior review has been addressed. The batch endpoint now has proper time range validation, consistent error handling, and logging parity with the existing /query endpoint. This PR is ready to merge and should deliver the intended performance improvement by reducing browser-to-server round trips for the traces dashboard. 🚀
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-scoped re-review — reviewers validated the fixes only, no new issues found.
1 participant
No description provided.