serve: log trace output directory on startup#929
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughThe PR adds trace directory discoverability to the startup sequence. A new ChangesTrace Directory Visibility
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/altimate/observability/trace-consumer.ts (1)
403-410: 💤 Low valueInformational: Trace directory not logged in config-failure fallback.
When
loadConfig()fails (lines 99-108),exportersremainsundefinedandgetTraceDirectory()returnsundefined, so no startup log is emitted. The warning"[tracing] failed to load config, using default tracer"indicates tracing is active, but the output directory is not surfaced.This is a minor observability gap, but fixing it would require determining the default
FileExporterdirectory at startup (currently determined lazily ingetOrCreateTrace()). The current behavior is acceptable since config failures should be rare and the warning provides some signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/observability/trace-consumer.ts` around lines 403 - 410, Startup logging omits the trace directory when loadConfig() fails because exporters is undefined and getTraceDirectory() returns undefined; to fix, determine and log the default FileExporter directory during startup fallback: update the fallback path in loadConfig() or trace consumer initialization so that when exporters is undefined you call the logic that computes the default directory (the same logic used by getOrCreateTrace()/FileExporter) and pass that value to Log.Default.info("[tracing] session traces", { directory }), ensuring getTraceDirectory() still returns a value even after a config load failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/altimate/observability/trace-consumer.ts`:
- Around line 403-410: Startup logging omits the trace directory when
loadConfig() fails because exporters is undefined and getTraceDirectory()
returns undefined; to fix, determine and log the default FileExporter directory
during startup fallback: update the fallback path in loadConfig() or trace
consumer initialization so that when exporters is undefined you call the logic
that computes the default directory (the same logic used by
getOrCreateTrace()/FileExporter) and pass that value to
Log.Default.info("[tracing] session traces", { directory }), ensuring
getTraceDirectory() still returns a value even after a config load failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfc4199b-d5da-4d33-864c-390006ceeed0
📒 Files selected for processing (1)
packages/opencode/src/altimate/observability/trace-consumer.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: block
The PR contains critical flaws in financial classification logic that risk misreporting DBUs and violating compliance requirements. The multiIf logic silently misclassifies unknown or null sql_tier values as 'serverless', which could lead to financial inaccuracies, and it bypasses mandatory finance stakeholder review per CLAUDE.md. These issues are confirmed by multiple agents and pose unacceptable risk to billing integrity.
15/15 agents completed · 233s · 4 findings (1 critical, 3 high, 0 medium)
Critical
- [code-reviewer, tech-lead] The multiIf logic incorrectly classifies all non-CLASSIC and non-PRO sql_tier values as 'serverless', including NULL or undefined values, which contradicts intent and risks financial misreporting. →
app/repositories/clickhouse_repositories/databricks/billing.py:577- 💡 Explicitly handle unknown/empty cases: use multiIf(JSONExtractString(usage.product_features, 'sql_tier') = 'CLASSIC', 'classic', JSONExtractString(usage.product_features, 'sql_tier') = 'PRO', 'pro', JSONExtractString(usage.product_features, 'sql_tier') IN ('SERVERLESS', ''), 'serverless', 'unknown') and log anomalies.
High
- [code-reviewer, tech-lead] Product filter expansion to 'SERVERLESS_SQL' assumes all such products have valid sql_tier metadata, but does not validate or handle NULL/empty sql_tier values, risking misclassification. →
app/repositories/clickhouse_repositories/databricks/billing.py:580- 💡 Add a condition to ensure sql_tier is not NULL or empty for 'SERVERLESS_SQL' entries, or document that this classification assumes valid metadata.
- [code-reviewer, tech-lead, pr-hygiene] PR modifies financial classification logic without required finance/ops stakeholder review as mandated by CLAUDE.md, creating compliance risk. →
app/repositories/clickhouse_repositories/databricks/billing.py:546- 💡 Add a comment or approval tag from a finance/ops stakeholder per CLAUDE.md requirements before merging.
- [tech-lead] Billing classification logic is implemented directly in the repository layer without alignment to a shared domain service, creating risk of duplication and inconsistency across endpoints. →
app/repositories/clickhouse_repositories/databricks/billing.py:574- 💡 Move warehouse type classification logic to a shared service (e.g., app/service/cost_classification.py) that both this repository and the by-product endpoint can import.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
2 participants
Summary
Logs the resolved trace output directory when the trace consumer starts.
This makes session tracing discoverable in
altimate serveby surfacing where trace files are being written at startup.Changes
TraceConsumer.getTraceDirectory()loadConfig()completesFileExporteris configuredExample log output:
Validation
bun run typecheckFixes #916
Summary by cubic
On startup,
altimate servenow logs the resolved session trace directory so you can quickly find where trace files are written. The log appears only when a FileExporter is configured.Written for commit a6d3617. Summary will update on new commits.
Summary by CodeRabbit