Skip to content

feat(storage): add resource span attributes for ACO ( App Centric Observability ) for async client#16151

Open
bajajneha27 wants to merge 15 commits into
googleapis:mainfrom
bajajneha27:509338299-async
Open

feat(storage): add resource span attributes for ACO ( App Centric Observability ) for async client#16151
bajajneha27 wants to merge 15 commits into
googleapis:mainfrom
bajajneha27:509338299-async

Conversation

@bajajneha27

Copy link
Copy Markdown
Contributor

No description provided.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Jun 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cache().EndFetch(bucket_name);
});

bg_tasks_.push_back(std::move(f));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There is a data race on bg_tasks_ because bg_tasks_.push_back is called without holding the mu_ mutex. Since MaybeTriggerBackgroundFetch can be called concurrently from multiple threads, and CleanupCompletedTasks also modifies bg_tasks_ under mu_, this concurrent modification of std::vector leads to undefined behavior and potential crashes. Protect the push_back call with mu_ just like in AsyncConnectionTracing.

  std::unique_lock<std::mutex> lk(mu_);
  bg_tasks_.push_back(std::move(f));

Comment on lines +81 to +83
} else if (result.status().code() == StatusCode::kPermissionDenied) {
cache().Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To be consistent with AsyncConnectionTracing and to handle any potentially unnormalized bucket names safely, normalize the bucket_name before constructing the fallback resource ID on kPermissionDenied errors.

    } else if (result.status().code() == StatusCode::kPermissionDenied) {
      auto const normalized = BucketMetadataCache::NormalizeBucketName(bucket_name);
      cache().Put(bucket_name, {"projects/_/buckets/" + normalized, "global"});
    }

Comment thread google/cloud/storage/internal/bucket_metadata_cache.h Outdated
Comment thread google/cloud/storage/internal/async/connection_tracing.cc Outdated
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.40853% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.18%. Comparing base (fd73bee) to head (9450ced).

Files with missing lines Patch % Lines
...cloud/storage/internal/async/connection_tracing.cc 35.00% 39 Missing ⚠️
.../cloud/storage/internal/tracing_connection_test.cc 95.12% 6 Missing ⚠️
...le/cloud/storage/internal/bucket_metadata_cache.cc 94.52% 4 Missing ⚠️
...oogle/cloud/storage/internal/tracing_connection.cc 98.31% 4 Missing ⚠️
...gle/cloud/storage/internal/bucket_metadata_cache.h 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16151      +/-   ##
==========================================
- Coverage   92.20%   92.18%   -0.02%     
==========================================
  Files        2264     2267       +3     
  Lines      208864   209376     +512     
==========================================
+ Hits       192579   193023     +444     
- Misses      16285    16353      +68     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bajajneha27 bajajneha27 marked this pull request as ready for review June 11, 2026 05:04
@bajajneha27 bajajneha27 requested review from a team as code owners June 11, 2026 05:04
@v-pratap v-pratap self-requested a review June 11, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant