Skip to content

feat: add size to object store tracing#5962

Merged
wjones127 merged 3 commits intolance-format:mainfrom
wjones127:tracing-object-store
Feb 19, 2026
Merged

feat: add size to object store tracing#5962
wjones127 merged 3 commits intolance-format:mainfrom
wjones127:tracing-object-store

Conversation

@wjones127
Copy link
Copy Markdown
Contributor

  • Adds size field to object store traces
  • Formats paths with .as_ref(), so they aren't wrapped in debug representation of Path.

@github-actions github-actions Bot added the enhancement New feature or request label Feb 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

PR Review: feat: add size to object store tracing

Overall this is a useful enhancement for observability. A few items to consider:

P1: Potential issue with get size reporting

In get(), the code records res.meta.size as the size field:

span.record("size", res.meta.size);

However, get() returns the entire object, but in get_opts() you record res.range.end - res.range.start:

span.record("size", res.range.end - res.range.start);

For get() without options, res.meta.size is the correct choice. But there's a subtle inconsistency: get_opts() uses the range size (bytes actually read), while get() uses metadata size. This is actually correct behavior since get() fetches the whole object, but it might be clearer to document this distinction or use res.range consistently for both.

P1: Multipart size recorded after completion only

The multipart upload size is recorded only in complete():

async fn complete(&mut self) -> OSResult<PutResult> {
    let res = self.target.complete().await?;
    self.write_span.record("size", self.write_size);
    Ok(res)
}

If abort() is called instead of complete(), the size won't be recorded. This may be intentional (aborted uploads aren't meaningful), but worth confirming.

P0: Missing debug_span import removal

The diff shows debug_span is removed from the import statement in favor of using tracing::Span::current(), but looking at the current file (line 18), the import still includes debug_span:

use tracing::{debug_span, instrument, Instrument, Span};

If this is no longer used, the unused import will likely cause a compiler warning. Please verify this was addressed.

@wjones127 wjones127 marked this pull request as ready for review February 18, 2026 00:34
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 18, 2026

Codecov Report

❌ Patch coverage is 99.46237% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-io/src/object_store/tracing.rs 99.46% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

wjones127 and others added 2 commits February 18, 2026 09:45
Tests verify span names and field values for put, get, get_range,
get_ranges, head, delete, copy, and put_multipart using InMemory as the
backing store.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@wjones127 wjones127 merged commit 9b0fa9a into lance-format:main Feb 19, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants