feat(storage): single span for ReadObject()
#14435
Conversation
Keep the span created for `ReadObject()` open until the download completes or is interrupted. This also groups resumed downloads and backoff spans under one umbrella.
product-auto-label
bot
added
the
api: storage
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14435 +/- ##
========================================
Coverage 93.59% 93.59%
========================================
Files 2313 2315 +2
Lines 206951 207118 +167
========================================
+ Hits 193686 193850 +164
- Misses 13265 13268 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @coryan)
| GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN | ||
|
|
||
| /** | ||
| * |
| EXPECT_CALL(*mock, Read) | ||
| .WillOnce([root] { | ||
| EXPECT_TRUE(ThereIsAnActiveSpan()); | ||
| EXPECT_EQ(opentelemetry::trace::Tracer::GetCurrentSpan()->GetContext(), |
There was a problem hiding this comment.
nit: this was common enough to warrant a matcher.
EXPECT_THAT(root, IsActive());
Here and below.
| } | ||
|
|
||
| bool TracingObjectReadSource::IsOpen() const { | ||
| auto scope = opentelemetry::trace::Scope(span_); |
There was a problem hiding this comment.
comment: this is probably good practice in case we add any instrumentation to lower layers. But just an FYI: that creating a scope does a heap allocation, so it is relatively expensive.
There was a problem hiding this comment.
Is that true even of Noop spans?
There was a problem hiding this comment.
Is that true even of Noop spans?
Yes. They do not check whether it is a valid span before pushing to the context stack (and creating a token to track it).
Keep the span created for
ReadObject()open until the downloadcompletes or is interrupted. This also groups resumed downloads and
backoff spans under one umbrella.
Fixes #11393
This change is