use http.response.status_code for traces status filter#407
use http.response.status_code for traces status filter#407
Conversation
Switch from span_status_code to http.response.status_code for determining trace status. Bun returns bigint columns as strings, so formatStatus parses the string to a number before comparison. NULL HTTP status (no response) is treated as error consistently. Closes #406 Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes update trace status filtering from OTel span status codes to HTTP response status codes, restoring 4xx client error visibility in error filters. Documentation guidelines added clarifying Bun.sql's bigint-to-string behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude add some knowledge to your AGENTS.md so that you remember that Bun.sql returns bigint numbers as string. |
|
Claude finished @heiwen's task in 35s —— View job Adding Bun.sql knowledge to AGENTS.md
Added a note under Key Technologies & Use Cases in |
Co-authored-by: Heinrich Wendel <heiwen@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1740b89b1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/traces/service.ts (1)
183-213:⚠️ Potential issue | 🟠 Major
getSpanslabels all non-HTTP internal spans as"error"due to missing HTTP status code.In
getSpans(line 246), all spans are assigned status viaformatStatus(row.http_status_code). TheformatStatusfunction returns"error"when the HTTP status code is null or undefined. Since internal gen_ai spans (embeddings, tool calls, reasoning steps, etc.) do not carry thehttp.response.status_codeattribute, they will all be labeled as"error"in the span detail view.While the PR's intent to treat NULL HTTP status as error is reasonable for
listTraces(where the root span is HTTP-level), this breaksgetSpanswhere most spans are internal operations without HTTP responses. The current implementation lacks any fallback to actual OpenTelemetry span status.Either restore
span_status_codeas the status source forgetSpans, or combine both signals: treat a span as"error"only ifhttp_status_code >= 400ORspan_status_codeindicates error; otherwise"ok".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/traces/service.ts` around lines 183 - 213, getSpans currently uses formatStatus(row.http_status_code) which treats NULL as "error", mislabeling internal non-HTTP spans; update getSpans to prefer the OpenTelemetry span status (span_status_code / span_status_message) and only use http_status_code when present and >= 400. Specifically, include "span_status_code" in the SELECT (or ensure the existing span_status_code field is returned) and change the status resolution in getSpans to: if http_status_code != null then use formatStatus(http_status_code) treating >=400 as error, else fall back to span_status_code (or combine: mark error if http_status_code >=400 OR span_status_code indicates ERROR); adjust calls to formatStatus or add a new helper to reflect this combined logic.
🧹 Nitpick comments (3)
apps/api/src/modules/traces/service.ts (1)
88-94: Status filter LGTM; consider an index/verify column cardinality.Filtering on
span_attributes.http.response.status_codealongside the existing time-range + agent/branch/org predicates is correct and matches the newformatStatussemantics. TheIS NULL OR >= 400branch correctly preserves visibility of 4xx and of spans that never produced a response.One operational note: if traces volume is high, confirm GreptimeDB has this column indexed (or that the time-range + tag filters sufficiently prune before this predicate is evaluated). Otherwise status-filtered queries may scan considerably more than before.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/traces/service.ts` around lines 88 - 94, The status-filter logic using HTTP_STATUS_COL (the string constant for "span_attributes.http.response.status_code") and the statusFilterSql branches is fine; now verify DB performance by ensuring the column is indexed or sufficiently selective with existing time-range + agent/branch/org predicates—if not, add an index (or a composite index including the time/tags) on span_attributes.http.response.status_code or adjust query plans; also check the column cardinality and query explain plans for the functions that build statusFilterSql to confirm the predicate doesn't force large table scans.apps/api/src/modules/traces/utils.ts (2)
41-41: Parse bigint strings withNumber.parseInt/parseFloatfor clarity, or handlebiginttype explicitly.Bun.sql may also hand back actual
bigintvalues in some driver paths;Number(bigint)works buttypeofcheck here only branches onstringvs falls through. Current behavior is correct for the documented case (string), but consider being explicit:♻️ Optional explicit handling
- const code = typeof httpStatusCode === "string" ? Number(httpStatusCode) : httpStatusCode; + const code = + typeof httpStatusCode === "string" || typeof httpStatusCode === "bigint" + ? Number(httpStatusCode) + : httpStatusCode;As per coding guidelines: "Always parse string values from Bun.sql bigint columns to numbers before performing arithmetic or numeric comparisons".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/traces/utils.ts` at line 41, The current assignment for the variable named code (const code = typeof httpStatusCode === "string" ? Number(httpStatusCode) : httpStatusCode) only distinguishes strings and falls through for other types; update this to explicitly handle string and bigint types: if httpStatusCode is a string parse it with Number.parseInt or Number.parseFloat as appropriate, if it is a bigint convert it to a Number (or handle overflow/edge-cases per project rules), otherwise keep the existing numeric value—modify the expression that sets code (referencing httpStatusCode and the const code) to perform these explicit type branches.
40-46: Dead branch in return type:"unknown"is no longer reachable.
formatStatusnow returns only"ok"or"error"— the"unknown"arm was removed but is still declared in the return type. Narrow the signature so callers (and the Elysia/Eden schema surface downstream) reflect reality.♻️ Proposed narrowing
-export function formatStatus(httpStatusCode: unknown): "ok" | "error" | "unknown" { +export function formatStatus(httpStatusCode: unknown): "ok" | "error" { const code = typeof httpStatusCode === "string" ? Number(httpStatusCode) : httpStatusCode; if (typeof code === "number" && Number.isFinite(code)) { return code >= 400 ? "error" : "ok"; } return "error"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/traces/utils.ts` around lines 40 - 46, The declared return type for formatStatus is inaccurate: update the function signature to remove the unreachable "unknown" variant and declare the return type as "ok" | "error" only; keep the implementation (casting httpStatusCode, checking Number.isFinite and >=400) but change the function's declared return type to match its actual outputs so downstream schema and callers reflect the narrowed type (function name: formatStatus).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/modules/traces/service.ts`:
- Around line 183-213: getSpans currently uses
formatStatus(row.http_status_code) which treats NULL as "error", mislabeling
internal non-HTTP spans; update getSpans to prefer the OpenTelemetry span status
(span_status_code / span_status_message) and only use http_status_code when
present and >= 400. Specifically, include "span_status_code" in the SELECT (or
ensure the existing span_status_code field is returned) and change the status
resolution in getSpans to: if http_status_code != null then use
formatStatus(http_status_code) treating >=400 as error, else fall back to
span_status_code (or combine: mark error if http_status_code >=400 OR
span_status_code indicates ERROR); adjust calls to formatStatus or add a new
helper to reflect this combined logic.
---
Nitpick comments:
In `@apps/api/src/modules/traces/service.ts`:
- Around line 88-94: The status-filter logic using HTTP_STATUS_COL (the string
constant for "span_attributes.http.response.status_code") and the
statusFilterSql branches is fine; now verify DB performance by ensuring the
column is indexed or sufficiently selective with existing time-range +
agent/branch/org predicates—if not, add an index (or a composite index including
the time/tags) on span_attributes.http.response.status_code or adjust query
plans; also check the column cardinality and query explain plans for the
functions that build statusFilterSql to confirm the predicate doesn't force
large table scans.
In `@apps/api/src/modules/traces/utils.ts`:
- Line 41: The current assignment for the variable named code (const code =
typeof httpStatusCode === "string" ? Number(httpStatusCode) : httpStatusCode)
only distinguishes strings and falls through for other types; update this to
explicitly handle string and bigint types: if httpStatusCode is a string parse
it with Number.parseInt or Number.parseFloat as appropriate, if it is a bigint
convert it to a Number (or handle overflow/edge-cases per project rules),
otherwise keep the existing numeric value—modify the expression that sets code
(referencing httpStatusCode and the const code) to perform these explicit type
branches.
- Around line 40-46: The declared return type for formatStatus is inaccurate:
update the function signature to remove the unreachable "unknown" variant and
declare the return type as "ok" | "error" only; keep the implementation (casting
httpStatusCode, checking Number.isFinite and >=400) but change the function's
declared return type to match its actual outputs so downstream schema and
callers reflect the narrowed type (function name: formatStatus).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4665a2a1-5505-402b-8a88-43642661de88
📒 Files selected for processing (3)
AGENTS.mdapps/api/src/modules/traces/service.tsapps/api/src/modules/traces/utils.ts
…eboai/hebo into claude-issue-406-20260417-1005
|
Open Issue: if we don't set span_status_code, there is also no error message |
1 participant
Use
http.response.status_codeinstead ofspan_status_codefor traces status filtering and formatting. Bun returns bigint columns as strings, soformatStatusparses the string to a number before comparison. NULL HTTP status (no response) is treated as error consistently.Closes #406
Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes