Trace improvements#2502
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 18adcb0 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 |
|
Improves trace observability by fixing the API (
Traces UI (
|
There was a problem hiding this comment.
Clean PR. Backend changes correctly fix the agent name discrepancy in span attributes, and the error filter + newline rendering changes are well-plumbed. One accessibility nit on the toggle button.
| <Button | ||
| variant="gray-outline" | ||
| size="sm" | ||
| onClick={() => setHasErrorsFilter(!hasErrors)} | ||
| className={ | ||
| hasErrors | ||
| ? 'border-destructive/40 text-destructive hover:bg-destructive/5 hover:text-destructive' | ||
| : '' | ||
| } | ||
| > | ||
| <CircleAlert className="h-3.5 w-3.5" /> | ||
| Errors only | ||
| </Button> |
There was a problem hiding this comment.
This button acts as a toggle — add aria-pressed={hasErrors} so screen readers announce the on/off state.
| <Button | |
| variant="gray-outline" | |
| size="sm" | |
| onClick={() => setHasErrorsFilter(!hasErrors)} | |
| className={ | |
| hasErrors | |
| ? 'border-destructive/40 text-destructive hover:bg-destructive/5 hover:text-destructive' | |
| : '' | |
| } | |
| > | |
| <CircleAlert className="h-3.5 w-3.5" /> | |
| Errors only | |
| </Button> | |
| <Button | |
| variant="gray-outline" | |
| size="sm" | |
| onClick={() => setHasErrorsFilter(!hasErrors)} | |
| aria-pressed={hasErrors} | |
| className={ | |
| hasErrors | |
| ? 'border-destructive/40 text-destructive hover:bg-destructive/5 hover:text-destructive' | |
| : '' | |
| } | |
| > | |
| <CircleAlert className="h-3.5 w-3.5" /> | |
| Errors only | |
| </Button> |
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
traces/page.tsx:248-260Missingaria-pressedfor toggle button accessibility - 🟡 Minor:
.changeset/moody-deer-own.md:6Changeset message too vague per AGENTS.md guidelines
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
Agent.ts:207agentNameoptionality may causeundefinedspan attributes for nested sub-agents
💡 APPROVE WITH SUGGESTIONS
Summary: This PR cleanly implements three trace improvements: an error filter for conversations, newline rendering in message content, and an agent name span attribute fix. The changes follow established patterns (e.g., hasErrors filter mirrors the existing agentId filter). The two minor issues are: (1) missing aria-pressed on the toggle button for accessibility, and (2) the changeset message being too vague. The optional agentName field may cause undefined span attributes for nested sub-agents, but this is a consideration rather than a blocker since the primary use case (top-level agent spans) is correctly handled.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
signoz-stats.ts |
Missing unit tests for hasErrors filter logic |
Pre-existing condition — no unit tests exist for this module, not introduced by this PR |
generateTaskHandler.ts:200 |
Missing test for agentName propagation |
Valid concern but test infrastructure setup is out of PR scope; flagged in Consider section instead |
Agent.ts:2412 |
Missing test for span attribute change | Pre-existing test gap, not introduced by this PR |
timeline-item.tsx:86 |
Newline transformation runs on every render | Negligible performance impact for typical list sizes; pattern is acceptable |
render-panel-content.tsx:264 |
String transformation at render time | Panel only renders when selected; acceptable pattern |
signoz-stats.ts filter |
INFO: follows established patterns | Positive finding, not an issue |
use-traces-query-state.ts |
INFO: follows established patterns | Positive finding, not an issue |
chat.ts:326 |
INFO: span attribute naming consistent | Positive finding, not an issue |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-frontend |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-consistency |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-types |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 16 | 0 | 0 | 0 | 3 | 0 | 13 |
Note: Many consistency and test findings were positive confirmations ("follows existing patterns") or pre-existing gaps not introduced by this PR.
| tenantId: string; | ||
| projectId: string; | ||
| agentId: string; | ||
| agentName?: string; |
There was a problem hiding this comment.
💭 Consider: Consider whether agentName should have a fallback
Issue: agentName is optional, but nested sub-agent relations (e.g., subAgentRelations, delegateRelations in generateTaskHandler.ts lines 210-262) don't propagate this field. When a sub-agent creates its span, agent.name will be undefined.
Why: While the top-level agent correctly sets agentName: currentAgent.name at line 200, nested agents constructed from relations don't include agentName. This could result in missing agent.name span attributes for sub-agent traces, affecting SigNoz filtering/grouping.
Fix: Consider adding a fallback at the span attribute level:
'agent.name': this.config.agentName ?? this.config.name,Or ensure agentName is propagated in nested relation configs.
Refs:
…s/page.tsx Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Delta Review — Prior Feedback Addressed
This is a re-review scoped to the 2 commits since the last automated review. Both issues raised in the prior review have been correctly addressed:
| Prior Issue | Status | Fix |
|---|---|---|
| 🟡 Changeset message too vague | ✅ Fixed | Updated to "Add hasErrors filter to traces page and fix agent.name span attribute" — follows AGENTS.md guidelines (sentence case, action verb, specific) |
🟡 Missing aria-pressed for accessibility |
✅ Fixed | Added aria-pressed={hasErrors} and aria-hidden="true" on the icon |
No new issues were introduced in the delta.
✅ APPROVE
Summary: All prior review feedback has been addressed. The changeset message now clearly describes the changes, and the error filter toggle button is accessible to screen reader users. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| N/A — delta-only re-review | — | — | — | — | — | — | — |
Note: Sub-reviewers were not dispatched for this re-review because the delta contained only fixes for prior review feedback with no new logic changes.
|
No documentation updates required for this PR. The changes are:
These are UI/internal improvements that don't require user-facing documentation updates. |
|
@claude check this |
|
Claude finished @robert-inkeep's task in 4m 22s —— View job Investigating double-encoding of message content in traces
You're right — the Data flow
Root causeThe escaping happens in the ClickHouse/SigNoz storage layer. ClickHouse stores string values with C-style escape sequences — actual newlines ( This is not a double-encoding bug in our code — our API correctly sets plain text attributes. The escaping is introduced by the storage/query layer we don't control. Recommended fixInstead of scattered Option A — Fix in function unescapeCStyleString(s: string): string {
return s.replace(/\\([nrtv0'"\\])/g, (_, ch) => {
const map: Record<string, string> = {
n: '\n', r: '\r', t: '\t', v: '\v',
'0': '\0', "'": "'", '"': '"', '\\': '\\',
};
return map[ch] ?? ch;
});
}
function getString(span: SigNozListItem, key: string, fallback = ''): string {
const v = getField(span, key);
const raw = typeof v === 'string' ? v : v == null ? fallback : String(v);
return unescapeCStyleString(raw);
}This is the cleanest approach because it fixes the problem at the boundary where SigNoz data enters our system. All downstream consumers (timeline items, panel content, any future components) would get properly unescaped strings. Then the two Risk: This would unescape all string attributes, not just message content. For most attributes (span IDs, agent names, timestamps) this is a no-op since they don't contain escape sequences. But worth verifying that no attribute intentionally stores literal Option B — If you want to be more targeted, apply messageContent: unescapeCStyleString(getString(span, SPAN_KEYS.MESSAGE_CONTENT, '')),This is safer but means you'd need to add it to every string field that could contain user-authored text. |
|
@claude the content though for my spans for ai assistant response look fine though, the text looks right with all the right line breaks |
|
Claude finished @robert-inkeep's task in 3m 26s —— View job Investigating span content encoding for AI assistant responses
Good observation — that's actually useful for narrowing down where the escaping happens. Here's what I found: Both
If your spans show the right line breaks in SigNoz's own UI, that confirms the escaping is not in ClickHouse storage — it's happening somewhere in the SigNoz API response → our conversation route pipeline. The previous analysis attributing it to ClickHouse was likely wrong. Why AI assistant responses look fine in the traces UI despite the same escaping: they're rendered through The recommendation from the prior analysis still holds — fix it once at the The remaining question is whether the escaping is:
You could verify by logging the raw axios response for a conversation that has newlines in |
filter conversations with errors
message contents should render new lines
agent name discrepancy