feat(rivetkit): traces #4037
feat(rivetkit): traces #4037
Conversation
|
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.
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
More templates
@rivetkit/virtual-websocket
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/traces
@rivetkit/workflow-engine
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| import { AsyncLocalStorage } from "node:async_hooks"; | ||
| import { Buffer } from "node:buffer"; | ||
| import { randomBytes } from "node:crypto"; | ||
| import { performance } from "node:perf_hooks"; | ||
| import { decode as decodeCbor, encode as encodeCbor } from "cbor-x"; | ||
| import { pack, unpack } from "fdb-tuple"; | ||
| import { | ||
| CHUNK_VERSIONED, | ||
| CURRENT_VERSION, | ||
| READ_RANGE_VERSIONED, | ||
| encodeRecord, | ||
| type ActiveSpanRef, | ||
| type Attributes, | ||
| type Chunk, | ||
| type KeyValue, | ||
| type ReadRangeWire, | ||
| type Record as TraceRecord, | ||
| type RecordBody, | ||
| type SpanEnd, | ||
| type SpanEvent, | ||
| type SpanId, | ||
| type SpanLink, | ||
| type SpanRecordKey, | ||
| type SpanSnapshot, | ||
| type SpanStart, | ||
| type SpanStatus, | ||
| SpanStatusCode, | ||
| type SpanUpdate, | ||
| type StringId, | ||
| type TraceId, | ||
| } from "../schemas/versioned.js"; | ||
| import { | ||
| anyValueFromJs, | ||
| hexFromBytes, | ||
| type OtlpExportTraceServiceRequestJson, | ||
| type OtlpKeyValue, | ||
| type OtlpResource, | ||
| type OtlpSpan, | ||
| type OtlpSpanEvent, | ||
| type OtlpSpanLink, | ||
| type OtlpSpanStatus, | ||
| } from "./otlp.js"; | ||
| import type { | ||
| EndSpanOptions, | ||
| EventOptions, | ||
| ReadRangeOptions, | ||
| ReadRangeResult, | ||
| SpanHandle, | ||
| SpanStatusInput, | ||
| StartSpanOptions, | ||
| Traces, | ||
| TracesDriver, | ||
| TracesOptions, | ||
| UpdateSpanOptions, | ||
| } from "./types.js"; |
There was a problem hiding this comment.
The imports need to be sorted alphabetically. Biome linter expects imports to be sorted in a specific order. Run 'biome check --write .' to automatically fix this issue.
Spotted by Graphite Agent (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
NathanFlurry
force-pushed
the
traces-storage
branch
from
846c70c to
7504a0b
Compare
NathanFlurry
force-pushed
the
01-21-feat_rivetkit_integrate_workflows_in_to_actors
branch
from
2bdce1c to
9524546
Compare
jog1t
force-pushed
the
01-21-feat_rivetkit_integrate_workflows_in_to_actors
branch
from
9524546 to
e9cb986
Compare
PR Review: feat(rivetkit): tracesThis is an impressive implementation of a compact, high-performance tracing system for RivetKit. The design is well-thought-out with excellent documentation and a clear spec. ✅ StrengthsArchitecture & Design
Implementation Quality
🔍 Critical Issues1. Potential Memory Leak in Write ChainLocation: traces.ts:559-567 The writeChain promise chain keeps growing indefinitely without cleanup. If errors occur, they're not handled and could cause unhandled rejections. Recommendation: Add error handling to the promise chain 2. Race Condition in FlushLocation: traces.ts:777-784 resetChunkState is called before writeChain completes. If new records are appended during the write, they could be lost. Recommendation: Wait for writes before resetting state 🐛 Bugs to Fix3. Integer Overflow in Chunk IDLocation: traces.ts:240-244 No validation that chunkId doesn't exceed MAX_CHUNK_ID. This could cause data corruption. 4. Unbounded Attribute ValuesLocation: traces.ts:293-320 No size limit on attribute values. A buggy actor could store arbitrarily large objects. ⚡ Performance Improvements5. Inefficient Array SearchLocation: traces.ts:1139-1144 Linear search through pendingChunks array could become slow. Consider using a Map for O(1) lookups. 6. Repeated String InterningLocation: traces.ts:259-268 Consider a global string cache for common attribute keys. 🧪 Testing Gaps7. Missing Edge Case Tests
8. No Integration Test with ActorDriverTests use InMemoryTracesDriver, but ActorTracesDriver isn't tested. 📋 Summary
Before Merge:
Post-Merge:
Overall, this is high-quality work with excellent design. The identified issues are mostly edge cases and hardening opportunities. Great job! 🎉 |
No description provided.