feat(storage): add resource span attributes for ACO ( App Centric Observability ) for async client#16151
feat(storage): add resource span attributes for ACO ( App Centric Observability ) for async client#16151bajajneha27 wants to merge 21 commits into
Conversation
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 a BucketMetadataCache to cache bucket metadata (ID and location) and uses it to enrich OpenTelemetry spans with destination resource attributes in both TracingConnection and AsyncConnectionTracing. When metadata is not cached, a background fetch is triggered. The review feedback highlights several important issues: a critical data race on bg_tasks_ in TracingConnection::MaybeTriggerBackgroundFetch due to a lack of mutex protection during push_back, potential undefined behavior in BucketMetadataCache::Put if max_size is initialized to 0, unnormalized bucket names on permission denied errors in TracingConnection, and performance concerns regarding the expensive creation of a new StorageConnection on every background fetch in AsyncConnectionTracing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16151 +/- ##
========================================
Coverage 92.29% 92.29%
========================================
Files 2221 2221
Lines 207372 207585 +213
========================================
+ Hits 191386 191595 +209
- Misses 15986 15990 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9450ced to
db490d9
Compare
4244a97 to
0f627af
Compare
fa71a12 to
f18bad4
Compare
| static BucketMetadataCache& Singleton(); | ||
|
|
||
| static std::string NormalizeBucketName(std::string const& bucket) { | ||
| auto const prefix = std::string("projects/_/buckets/"); |
There was a problem hiding this comment.
If a project identifier is present in bucket (for instance, projects/123456/buckets/my-bucket), this function will not normalize it.
| -> StatusOr<google::storage::v2::Bucket> { | ||
| auto result = f.get(); | ||
| internal::DetachOTelContext(oc); | ||
| if (result.ok()) { |
There was a problem hiding this comment.
If the AsyncConnectionTracing object is destroyed before the future resolves, invoking cache().MaybeInvalidate(...) or calling EnrichSpan member method inside the callback will result in a use-after-free crash.
| return internal::EndSpan(std::move(span), | ||
| impl_->InsertObject(std::move(p))); | ||
| return impl_->InsertObject(std::move(p)) | ||
| .then([oc = opentelemetry::context::RuntimeContext::GetCurrent(), |
There was a problem hiding this comment.
Do we need these lines? The EndSpan template overload that takes a future parameter already handles this context detachment automatically under the hood. Same comment for ComposeObject, and DeleteObject methods.
…ervability ) for async client
f18bad4 to
ab7a6df
Compare
| impl_->GetBucket(std::move(params)) | ||
| .then([cache = cache_, bucket_name, guard = std::move(guard)]( | ||
| future<StatusOr<google::storage::v2::Bucket>> f) { | ||
| auto metadata = f.get(); |
There was a problem hiding this comment.
nit: pls spell out the type for readability.
| } else if (metadata.status().code() == | ||
| StatusCode::kPermissionDenied) { | ||
| BucketCacheEntry entry{ | ||
| "projects/_/buckets/" + |
There was a problem hiding this comment.
nit: let's make this a class-level constant?
| impl_->DeleteObject(std::move(p))); | ||
| } | ||
|
|
||
| std::shared_ptr<storage::AsyncRewriterConnection> RewriteObject( |
There was a problem hiding this comment.
RewriteObject is missing span enrichment. The actual RPCs and span creation happen downstream in rewriter_connection_tracing.cc, so it might be better to fix in a follow-up PR.
2 participants
No description provided.