Conversation
f1d1973 to
f7a0b35
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8b2cff410
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2035d4a86c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
57dfa58 to
d38984a
Compare
8b1aab0 to
8276414
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 827641417b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Replace the OpenRangeReader return type io.ReadCloser with a context-aware RangeReader and collapse scattered reader wrappers (offset/section/capture/ observable/decompress) into a single io_wrappers.go. Drops the cacheWriteThroughReader in favor of a reusable captureReader with optional drain-on-close for codecs that stop short of EOF on their source.
d38984a to
082285c
Compare
8276414 to
605a3f5
Compare
75f2087 to
506c3e7
Compare
When NewDecompressingReader fails, closing the captureReader drained the raw stream into its buffer and persisted those bytes to NFS, poisoning the .frm cache on a failed miss. Close raw directly on the error path to bypass the drain+writeback side effect.
Adds an orchestrator.read.* metric family with consistent attributes
(file_type/source/codec/outcome) covering each stage of a read, plus
per-layer chunker and build-file timers, so dashboards can attribute
latency end-to-end from sandbox-visible read to backend fetch.
Does not remove any of the prior metrics, this will be done separately
after the dashboards are updated.
Metrics:
- orchestrator.file.read_at build.File.ReadAt — per-fault
unit, aggregates all underlying
mappings into one record
- orchestrator.chunk.slice Chunker.Slice — per per-mapping
unit, source=mmap on cache hit
else the backend that served
- orchestrator.read.open OpenRangeReader (open / TTFB)
- orchestrator.read.read source-read wall, compressed bytes
- orchestrator.read.decompress decompress CPU + uncompressed bytes
- orchestrator.read.fetch total fetch wall + bytes delivered
- orchestrator.read.writeback NFS cache writeback wall + bytes
- orchestrator.read.pipeline.efficiency fetch / (open+read+decompress)
- orchestrator.read.cache NFS hit/miss/writeback events
- orchestrator.read.inflight concurrent fetches gauge
Spans:
- chunk.fetch runFetch goroutine span
506c3e7 to
ba1e232
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| func StartInflight(ctx context.Context, attrs metric.MeasurementOption) func() { | ||
| readInflight.Add(ctx, 1, attrs) | ||
|
|
||
| return func() { readInflight.Add(ctx, -1, attrs) } | ||
| } |
There was a problem hiding this comment.
I'll just add the traditional warning here: gauges are rarely the right answer, and often misleading. It might be safer to record "reads begun" and "reads finished" as counters. Alternately, recording both would be reasonable.
TL;DR: gauges only show you a point-in-time every n seconds (30 or 60 by default, depending on system), no way to judge what happened in between.
As an example of what could be hidden by a gauge:
- a process increments and decrements that gauge 1000 times in 5 seconds, in between two readings. The gauge reports 0 at each moment in time, no indication of the flurry of activity.
- 50 reads are started/stopped every second. The gauge shows "50 concurrent reads" but what really happened was that there were 50 reads * 30 seconds = 1500 reads total; no way to differentiate that from "50 sustained reads over 30 seconds"
You can nearly always the gauge data from a counter; you can't get the counter data from the gauge. If point-in-time concurrency is valuable, let's keep it; otherwise increase(total_read_count[$__interval]) tells us "how many reads were started over this period, averaged per second"; not quite the same thing, but similar in terms of diagnostic value.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Adds to #2570 which should be merged first.
Adds an orchestrator.read.* metric family with consistent attributes
(file_type/source/codec/outcome) covering each stage of a read, plus
per-layer chunker and build-file timers, so dashboards can attribute
latency end-to-end from sandbox-visible read to backend fetch.
Does not remove any of the prior metrics, this will be done separately
after the dashboards are updated.
Metrics:
orchestrator.file.read_atbuild.File.ReadAt - per-fault unit, aggregates all underlying mappings into one recordorchestrator.chunk.sliceChunker.Slice - per per-mapping unit, source=mmap on cache hit else the backend that servedorchestrator.read.open- OpenRangeReader (open / TTFB)orchestrator.read.read- source-read wall time, raw bytesorchestrator.read.decompress- decompress wall time + uncompressed bytesorchestrator.read.fetch- total fetch wall time + uncompressed bytes deliveredorchestrator.read.writeback- NFS cache writeback wall + bytesorchestrator.read.pipeline.efficiency- fetch / (open+read+decompress)orchestrator.read.cache- NFS hit/miss/writeback eventsorchestrator.read.inflight- concurrent fetches gaugeSpans:
chunk.fetch- runFetch goroutine span