feat(storage): add telemetry for pre-warmed ranges in ObjectDescriptorImpl - #16323
feat(storage): add telemetry for pre-warmed ranges in ObjectDescriptorImpl#16323kalragauri wants to merge 3 commits into
Conversation
…nges in ObjectDescriptorImpl
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tracking and telemetry for the cache status ("HIT", "MISS", "EVICTED") of pre-warmed ranges in ObjectDescriptorImpl, recording it as an OpenTelemetry span attribute. The review feedback suggests limiting the size of the new evicted_ranges_ set to prevent unbounded memory growth, and optimizing performance by using absl::string_view instead of std::string to avoid unnecessary copies of the cache status.
kalragauri
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16323 +/- ##
=======================================
Coverage 92.24% 92.24%
=======================================
Files 2227 2227
Lines 209234 209247 +13
=======================================
+ Hits 192999 193013 +14
+ Misses 16235 16234 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| EnrichSpan(*span, p.options, p.read_spec.bucket()); | ||
| if (p.options.has<ReadRangesOption>()) { | ||
| auto const& ranges = p.options.get<ReadRangesOption>(); | ||
| span->SetAttribute("fast_open_ranges", ranges.size()); |
There was a problem hiding this comment.
Generally, we follow the naming convention with . or with - .
So, maybe something like gl-cpp.open.ranges.size ?
| // Check if this range matches a pre-warmed range. | ||
| auto cache_key = std::make_pair(p.start, p.length); | ||
| auto cache_it = prewarmed_ranges_.find(cache_key); | ||
| absl::string_view cache_status = "MISS"; |
There was a problem hiding this comment.
nit: Consider making it an enum.
| max_prewarmed_buffer_size_) { | ||
| // Evict the range if it exceeds the pacing limit. | ||
| total_prewarmed_bytes_buffered_ -= unclaimed_it->second.bytes_buffered; | ||
| if (evicted_ranges_.size() < 1000) { |
There was a problem hiding this comment.
nit: Can you please put a comment explaining this?
| future<ObjectDescriptorReader::ReadResponse> Read() override { | ||
| auto span = internal::MakeSpan("storage::AsyncConnection::ReadRange"); | ||
| if (!cache_status_.empty()) { | ||
| span->SetAttribute("fast_open_cache_status", std::string(cache_status_)); |
There was a problem hiding this comment.
Same comment as above for attribute name.
| auto span_catcher = InstallSpanCatcher(); | ||
| auto impl = std::make_shared<ReadRange>(10000, 30); | ||
| auto reader = MakeTracingObjectDescriptorReader(impl); | ||
| auto reader = |
There was a problem hiding this comment.
Can we test the attribute that we just set?
2 participants
This PR adds OpenTelemetry instrumentation for multi-range pre-warming requests in the GCS async client.