From 98770a7ade4b0ab41a0062a4246d9d9110aaba5b Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Wed, 23 Sep 2026 14:33:21 +0000 Subject: [PATCH 1/4] fix(storage): add App Hub storage.googleapis.com prefix to destination.id --- .../internal/async/connection_tracing.cc | 4 +-- .../internal/async/connection_tracing_test.cc | 32 +++++++++---------- .../storage/internal/bucket_metadata_cache.cc | 2 +- .../internal/bucket_metadata_cache_test.cc | 12 +++---- .../storage/internal/tracing_connection.cc | 2 +- .../internal/tracing_connection_test.cc | 12 +++---- 6 files changed, 32 insertions(+), 32 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_tracing.cc b/google/cloud/storage/internal/async/connection_tracing.cc index 4b8b9e1b22859..9c08735eeb47f 100644 --- a/google/cloud/storage/internal/async/connection_tracing.cc +++ b/google/cloud/storage/internal/async/connection_tracing.cc @@ -291,13 +291,13 @@ class AsyncConnectionTracing : public storage::AsyncConnection { StatusOr metadata = f.get(); if (metadata.ok()) { BucketCacheEntry entry = BucketCacheEntry::FromLocation( - metadata->project() + "/buckets/" + + "//storage.googleapis.com/" + metadata->project() + "/buckets/" + BucketMetadataCache::NormalizeBucketName(bucket_name), metadata->location(), metadata->location_type()); cache->Put(bucket_name, std::move(entry)); } else if (metadata.status().code() == StatusCode::kPermissionDenied) { - BucketCacheEntry entry{ + BucketCacheEntry entry{ "//storage.googleapis.com/" + std::string(kProjectBucketPrefix) + BucketMetadataCache::NormalizeBucketName(bucket_name), kGlobalLocation}; diff --git a/google/cloud/storage/internal/async/connection_tracing_test.cc b/google/cloud/storage/internal/async/connection_tracing_test.cc index 588b486e1458a..4df2ca0f93c74 100644 --- a/google/cloud/storage/internal/async/connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/connection_tracing_test.cc @@ -564,10 +564,10 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { // 1st call populates the cache google::storage::v2::GetBucketRequest req; - req.set_name("projects/_/buckets/test-bucket"); + req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); auto res1 = connection->GetBucket({req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("projects/123456"); + bucket_meta.set_project("//storage.googleapis.com/projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p.set_value(make_status_or(std::move(bucket_meta))); @@ -577,7 +577,7 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { // 2nd call: RewriteObject uses cached bucket metadata for span enrichment google::storage::v2::RewriteObjectRequest rewrite_req; - rewrite_req.set_destination_bucket("projects/_/buckets/test-bucket"); + rewrite_req.set_destination_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); auto rewriter = connection->RewriteObject({rewrite_req, options}); auto r1 = rewriter->Iterate().get(); ASSERT_STATUS_OK(r1); @@ -590,7 +590,7 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -870,12 +870,12 @@ TEST(ConnectionTracing, GetBucketSpanEnrichment) { auto actual = MakeTracingAsyncConnection(std::move(mock)); google::storage::v2::GetBucketRequest req; - req.set_name("projects/_/buckets/test-bucket"); + req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); auto result = actual->GetBucket({std::move(req), options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("projects/123456"); + bucket_meta.set_project("//storage.googleapis.com/projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p.set_value(make_status_or(std::move(bucket_meta))); @@ -890,7 +890,7 @@ TEST(ConnectionTracing, GetBucketSpanEnrichment) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -921,7 +921,7 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { auto actual = MakeTracingAsyncConnection(std::move(mock)); google::storage::v2::DeleteObjectRequest req; - req.set_bucket("projects/_/buckets/test-bucket"); + req.set_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); req.set_object("test-object-1"); auto res1 = actual->DeleteObject({req, options}).then(expect_no_context); @@ -930,7 +930,7 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { // Complete the background GetBucket fetch to populate the cache google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("projects/123456"); + bucket_meta.set_project("//storage.googleapis.com/projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); bucket_promise.set_value(make_status_or(std::move(bucket_meta))); @@ -953,7 +953,7 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -978,10 +978,10 @@ TEST(ConnectionTracing, GetBucketMaybeInvalidateEvicts) { // 1st call populates the cache google::storage::v2::GetBucketRequest req; - req.set_name("projects/_/buckets/test-bucket"); + req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); auto res1 = actual->GetBucket({req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("projects/123456"); + bucket_meta.set_project("//storage.googleapis.com/projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p1.set_value(make_status_or(std::move(bucket_meta))); @@ -1016,11 +1016,11 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { // 1st call: GetBucket populates the cache google::storage::v2::GetBucketRequest bucket_req; - bucket_req.set_name("projects/_/buckets/test-bucket"); + bucket_req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); auto res_bucket = actual->GetBucket({bucket_req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("projects/123456"); + bucket_meta.set_project("//storage.googleapis.com/projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); bucket_p.set_value(make_status_or(std::move(bucket_meta))); @@ -1028,7 +1028,7 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { // 2nd call: DeleteObject fails with kNotFound (object missing) google::storage::v2::DeleteObjectRequest del_req; - del_req.set_bucket("projects/_/buckets/test-bucket"); + del_req.set_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); del_req.set_object("missing-object"); auto res_del1 = actual->DeleteObject({del_req, options}).then(expect_no_context); @@ -1054,7 +1054,7 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } diff --git a/google/cloud/storage/internal/bucket_metadata_cache.cc b/google/cloud/storage/internal/bucket_metadata_cache.cc index aa9f75b9ecf12..6dd0cb5e17f87 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache.cc @@ -35,7 +35,7 @@ BucketCacheEntry BucketCacheEntry::FromLocation( BucketCacheEntry BucketCacheEntry::FromMetadata( storage::BucketMetadata const& m) { return FromLocation( - "projects/" + std::to_string(m.project_number()) + "/buckets/" + m.name(), + "//storage.googleapis.com/projects/" + std::to_string(m.project_number()) + "/buckets/" + m.name(), m.location(), m.location_type()); } diff --git a/google/cloud/storage/internal/bucket_metadata_cache_test.cc b/google/cloud/storage/internal/bucket_metadata_cache_test.cc index b5e6eb2ebe799..4e33e57ad850a 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache_test.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache_test.cc @@ -43,32 +43,32 @@ TEST(BucketMetadataCacheTest, HitAndMiss) { BucketMetadataCache cache(10); EXPECT_FALSE(cache.Get("test-bucket").has_value()); - BucketCacheEntry entry{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; cache.Put("test-bucket", entry); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("projects/123/buckets/test-bucket")); + EXPECT_THAT(res->id, Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("us-central1")); } TEST(BucketMetadataCacheTest, PutUpdatesExisting) { BucketMetadataCache cache(10); - BucketCacheEntry entry1{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry1{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; cache.Put("test-bucket", entry1); - BucketCacheEntry entry2{"projects/456/buckets/test-bucket", "global"}; + BucketCacheEntry entry2{"//storage.googleapis.com/projects/456/buckets/test-bucket", "global"}; cache.Put("test-bucket", entry2); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("projects/456/buckets/test-bucket")); + EXPECT_THAT(res->id, Eq("//storage.googleapis.com/projects/456/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("global")); } TEST(BucketMetadataCacheTest, InvalidateAndClear) { BucketMetadataCache cache(10); - BucketCacheEntry entry{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; cache.Put("test-bucket", entry); EXPECT_TRUE(cache.Get("test-bucket").has_value()); diff --git a/google/cloud/storage/internal/tracing_connection.cc b/google/cloud/storage/internal/tracing_connection.cc index ca37f072453a7..48884c10d432b 100644 --- a/google/cloud/storage/internal/tracing_connection.cc +++ b/google/cloud/storage/internal/tracing_connection.cc @@ -78,7 +78,7 @@ void TracingConnection::MaybeTriggerBackgroundFetch( if (result.ok()) { cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); } else if (result.status().code() == StatusCode::kPermissionDenied) { - cache->Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"}); + cache->Put(bucket_name, {"//storage.googleapis.com/projects/_/buckets/" + bucket_name, "global"}); } }); } diff --git a/google/cloud/storage/internal/tracing_connection_test.cc b/google/cloud/storage/internal/tracing_connection_test.cc index 3cc75811ff639..0f6f3c7e731fc 100644 --- a/google/cloud/storage/internal/tracing_connection_test.cc +++ b/google/cloud/storage/internal/tracing_connection_test.cc @@ -158,7 +158,7 @@ TEST(TracingClientTest, CreateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -210,7 +210,7 @@ TEST(TracingClientTest, GetBucketMetadataSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -263,7 +263,7 @@ TEST(TracingClientTest, BucketMetadataCacheSuccess) { SpanNamed("storage::Client::DeleteObject"), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -338,7 +338,7 @@ TEST(TracingClientTest, UpdateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -388,7 +388,7 @@ TEST(TracingClientTest, PatchBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -511,7 +511,7 @@ TEST(TracingClientTest, LockBucketRetentionPolicySuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } From 924c58467970b2651e21c8caa3cc932bac6da869 Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 28 Sep 2026 06:05:16 +0000 Subject: [PATCH 2/4] test(storage): revert errant prefix applied to test metadata objects --- .../internal/async/connection_tracing_test.cc | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_tracing_test.cc b/google/cloud/storage/internal/async/connection_tracing_test.cc index 4df2ca0f93c74..3504ae9e01e5b 100644 --- a/google/cloud/storage/internal/async/connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/connection_tracing_test.cc @@ -564,10 +564,10 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { // 1st call populates the cache google::storage::v2::GetBucketRequest req; - req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); + req.set_name("projects/_/buckets/test-bucket"); auto res1 = connection->GetBucket({req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("//storage.googleapis.com/projects/123456"); + bucket_meta.set_project("projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p.set_value(make_status_or(std::move(bucket_meta))); @@ -577,7 +577,7 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { // 2nd call: RewriteObject uses cached bucket metadata for span enrichment google::storage::v2::RewriteObjectRequest rewrite_req; - rewrite_req.set_destination_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); + rewrite_req.set_destination_bucket("projects/_/buckets/test-bucket"); auto rewriter = connection->RewriteObject({rewrite_req, options}); auto r1 = rewriter->Iterate().get(); ASSERT_STATUS_OK(r1); @@ -870,12 +870,12 @@ TEST(ConnectionTracing, GetBucketSpanEnrichment) { auto actual = MakeTracingAsyncConnection(std::move(mock)); google::storage::v2::GetBucketRequest req; - req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); + req.set_name("projects/_/buckets/test-bucket"); auto result = actual->GetBucket({std::move(req), options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("//storage.googleapis.com/projects/123456"); + bucket_meta.set_project("projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p.set_value(make_status_or(std::move(bucket_meta))); @@ -921,7 +921,7 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { auto actual = MakeTracingAsyncConnection(std::move(mock)); google::storage::v2::DeleteObjectRequest req; - req.set_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); + req.set_bucket("projects/_/buckets/test-bucket"); req.set_object("test-object-1"); auto res1 = actual->DeleteObject({req, options}).then(expect_no_context); @@ -930,7 +930,7 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { // Complete the background GetBucket fetch to populate the cache google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("//storage.googleapis.com/projects/123456"); + bucket_meta.set_project("projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); bucket_promise.set_value(make_status_or(std::move(bucket_meta))); @@ -978,10 +978,10 @@ TEST(ConnectionTracing, GetBucketMaybeInvalidateEvicts) { // 1st call populates the cache google::storage::v2::GetBucketRequest req; - req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); + req.set_name("projects/_/buckets/test-bucket"); auto res1 = actual->GetBucket({req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("//storage.googleapis.com/projects/123456"); + bucket_meta.set_project("projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); p1.set_value(make_status_or(std::move(bucket_meta))); @@ -1016,11 +1016,11 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { // 1st call: GetBucket populates the cache google::storage::v2::GetBucketRequest bucket_req; - bucket_req.set_name("//storage.googleapis.com/projects/_/buckets/test-bucket"); + bucket_req.set_name("projects/_/buckets/test-bucket"); auto res_bucket = actual->GetBucket({bucket_req, options}).then(expect_no_context); google::storage::v2::Bucket bucket_meta; - bucket_meta.set_project("//storage.googleapis.com/projects/123456"); + bucket_meta.set_project("projects/123456"); bucket_meta.set_location("us-east1"); bucket_meta.set_location_type("regional"); bucket_p.set_value(make_status_or(std::move(bucket_meta))); @@ -1028,7 +1028,7 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { // 2nd call: DeleteObject fails with kNotFound (object missing) google::storage::v2::DeleteObjectRequest del_req; - del_req.set_bucket("//storage.googleapis.com/projects/_/buckets/test-bucket"); + del_req.set_bucket("projects/_/buckets/test-bucket"); del_req.set_object("missing-object"); auto res_del1 = actual->DeleteObject({del_req, options}).then(expect_no_context); From c301e573d34ac65f290ac133f5be4baa685ef2f5 Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 28 Sep 2026 06:38:31 +0000 Subject: [PATCH 3/4] fix(storage): apply App Hub prefix in async EnrichSpan path and clang-format --- .../internal/async/connection_tracing.cc | 11 ++++++---- .../internal/async/connection_tracing_test.cc | 12 +++++++---- .../storage/internal/bucket_metadata_cache.cc | 7 ++++--- .../internal/bucket_metadata_cache_test.cc | 21 +++++++++++++------ .../storage/internal/tracing_connection.cc | 4 +++- .../internal/tracing_connection_test.cc | 18 ++++++++++------ 6 files changed, 49 insertions(+), 24 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_tracing.cc b/google/cloud/storage/internal/async/connection_tracing.cc index 9c08735eeb47f..4405569c9a689 100644 --- a/google/cloud/storage/internal/async/connection_tracing.cc +++ b/google/cloud/storage/internal/async/connection_tracing.cc @@ -268,6 +268,8 @@ class AsyncConnectionTracing : public storage::AsyncConnection { private: static constexpr char kProjectBucketPrefix[] = "projects/_/buckets/"; static constexpr char kGlobalLocation[] = "global"; + // The App Hub / Cloud Asset Inventory full resource name prefix. + static constexpr char kResourceNamePrefix[] = "//storage.googleapis.com/"; BucketMetadataCache& cache() const { return *cache_; } @@ -291,14 +293,15 @@ class AsyncConnectionTracing : public storage::AsyncConnection { StatusOr metadata = f.get(); if (metadata.ok()) { BucketCacheEntry entry = BucketCacheEntry::FromLocation( - "//storage.googleapis.com/" + metadata->project() + "/buckets/" + + std::string(kResourceNamePrefix) + metadata->project() + + "/buckets/" + BucketMetadataCache::NormalizeBucketName(bucket_name), metadata->location(), metadata->location_type()); cache->Put(bucket_name, std::move(entry)); } else if (metadata.status().code() == StatusCode::kPermissionDenied) { - BucketCacheEntry entry{ "//storage.googleapis.com/" + - std::string(kProjectBucketPrefix) + + BucketCacheEntry entry{ + std::string(kResourceNamePrefix) + kProjectBucketPrefix + BucketMetadataCache::NormalizeBucketName(bucket_name), kGlobalLocation}; cache->Put(bucket_name, std::move(entry)); @@ -321,7 +324,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection { google::cloud::storage_experimental::OTelSpanEnrichmentOption>(); if (!enabled) return; auto entry = BucketCacheEntry::FromLocation( - bucket.project() + "/buckets/" + + std::string(kResourceNamePrefix) + bucket.project() + "/buckets/" + BucketMetadataCache::NormalizeBucketName(bucket_name), bucket.location(), bucket.location_type()); EnrichSpan(span, entry); diff --git a/google/cloud/storage/internal/async/connection_tracing_test.cc b/google/cloud/storage/internal/async/connection_tracing_test.cc index 3504ae9e01e5b..a7d38829bd1c8 100644 --- a/google/cloud/storage/internal/async/connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/connection_tracing_test.cc @@ -590,7 +590,8 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -890,7 +891,8 @@ TEST(ConnectionTracing, GetBucketSpanEnrichment) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -953,7 +955,8 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -1054,7 +1057,8 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } diff --git a/google/cloud/storage/internal/bucket_metadata_cache.cc b/google/cloud/storage/internal/bucket_metadata_cache.cc index 6dd0cb5e17f87..1fa321156ef54 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache.cc @@ -34,9 +34,10 @@ BucketCacheEntry BucketCacheEntry::FromLocation( BucketCacheEntry BucketCacheEntry::FromMetadata( storage::BucketMetadata const& m) { - return FromLocation( - "//storage.googleapis.com/projects/" + std::to_string(m.project_number()) + "/buckets/" + m.name(), - m.location(), m.location_type()); + return FromLocation("//storage.googleapis.com/projects/" + + std::to_string(m.project_number()) + "/buckets/" + + m.name(), + m.location(), m.location_type()); } std::string BucketMetadataCache::NormalizeBucketName( diff --git a/google/cloud/storage/internal/bucket_metadata_cache_test.cc b/google/cloud/storage/internal/bucket_metadata_cache_test.cc index 4e33e57ad850a..b0832cdac74da 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache_test.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache_test.cc @@ -43,32 +43,41 @@ TEST(BucketMetadataCacheTest, HitAndMiss) { BucketMetadataCache cache(10); EXPECT_FALSE(cache.Get("test-bucket").has_value()); - BucketCacheEntry entry{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); + EXPECT_THAT(res->id, + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("us-central1")); } TEST(BucketMetadataCacheTest, PutUpdatesExisting) { BucketMetadataCache cache(10); - BucketCacheEntry entry1{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry1{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry1); - BucketCacheEntry entry2{"//storage.googleapis.com/projects/456/buckets/test-bucket", "global"}; + BucketCacheEntry entry2{ + "//storage.googleapis.com/projects/456/buckets/test-bucket", "global"}; cache.Put("test-bucket", entry2); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("//storage.googleapis.com/projects/456/buckets/test-bucket")); + EXPECT_THAT(res->id, + Eq("//storage.googleapis.com/projects/456/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("global")); } TEST(BucketMetadataCacheTest, InvalidateAndClear) { BucketMetadataCache cache(10); - BucketCacheEntry entry{"//storage.googleapis.com/projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry); EXPECT_TRUE(cache.Get("test-bucket").has_value()); diff --git a/google/cloud/storage/internal/tracing_connection.cc b/google/cloud/storage/internal/tracing_connection.cc index 48884c10d432b..264d2bf516977 100644 --- a/google/cloud/storage/internal/tracing_connection.cc +++ b/google/cloud/storage/internal/tracing_connection.cc @@ -78,7 +78,9 @@ void TracingConnection::MaybeTriggerBackgroundFetch( if (result.ok()) { cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); } else if (result.status().code() == StatusCode::kPermissionDenied) { - cache->Put(bucket_name, {"//storage.googleapis.com/projects/_/buckets/" + bucket_name, "global"}); + cache->Put(bucket_name, + {"//storage.googleapis.com/projects/_/buckets/" + bucket_name, + "global"}); } }); } diff --git a/google/cloud/storage/internal/tracing_connection_test.cc b/google/cloud/storage/internal/tracing_connection_test.cc index 0f6f3c7e731fc..a8f8eb7355848 100644 --- a/google/cloud/storage/internal/tracing_connection_test.cc +++ b/google/cloud/storage/internal/tracing_connection_test.cc @@ -158,7 +158,8 @@ TEST(TracingClientTest, CreateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -210,7 +211,8 @@ TEST(TracingClientTest, GetBucketMetadataSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -263,7 +265,8 @@ TEST(TracingClientTest, BucketMetadataCacheSuccess) { SpanNamed("storage::Client::DeleteObject"), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -338,7 +341,8 @@ TEST(TracingClientTest, UpdateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -388,7 +392,8 @@ TEST(TracingClientTest, PatchBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -511,7 +516,8 @@ TEST(TracingClientTest, LockBucketRetentionPolicySuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "//storage.googleapis.com/projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } From 827a75aabdee3730d5ec8188f6190c6c528d50bd Mon Sep 17 00:00:00 2001 From: cpriti-os Date: Mon, 5 Oct 2026 06:09:03 +0000 Subject: [PATCH 4/4] refactor(storage): centralize App Hub resource name formatting Move the //storage.googleapis.com/ prefix and bucket resource name construction into BucketCacheEntry so the sync and async tracing paths share one implementation. Fall back to projects/_ when the project is empty or the project number is 0. --- .../internal/async/connection_tracing.cc | 18 ++++--------- .../storage/internal/bucket_metadata_cache.cc | 21 ++++++++++++--- .../storage/internal/bucket_metadata_cache.h | 20 ++++++++++++++ .../internal/bucket_metadata_cache_test.cc | 27 +++++++++++++++++++ .../storage/internal/tracing_connection.cc | 27 +++++++++---------- 5 files changed, 82 insertions(+), 31 deletions(-) diff --git a/google/cloud/storage/internal/async/connection_tracing.cc b/google/cloud/storage/internal/async/connection_tracing.cc index 4405569c9a689..dd03daaeb6781 100644 --- a/google/cloud/storage/internal/async/connection_tracing.cc +++ b/google/cloud/storage/internal/async/connection_tracing.cc @@ -267,9 +267,6 @@ class AsyncConnectionTracing : public storage::AsyncConnection { private: static constexpr char kProjectBucketPrefix[] = "projects/_/buckets/"; - static constexpr char kGlobalLocation[] = "global"; - // The App Hub / Cloud Asset Inventory full resource name prefix. - static constexpr char kResourceNamePrefix[] = "//storage.googleapis.com/"; BucketMetadataCache& cache() const { return *cache_; } @@ -293,18 +290,14 @@ class AsyncConnectionTracing : public storage::AsyncConnection { StatusOr metadata = f.get(); if (metadata.ok()) { BucketCacheEntry entry = BucketCacheEntry::FromLocation( - std::string(kResourceNamePrefix) + metadata->project() + - "/buckets/" + - BucketMetadataCache::NormalizeBucketName(bucket_name), + BucketCacheEntry::ResourceName(metadata->project(), + bucket_name), metadata->location(), metadata->location_type()); cache->Put(bucket_name, std::move(entry)); } else if (metadata.status().code() == StatusCode::kPermissionDenied) { - BucketCacheEntry entry{ - std::string(kResourceNamePrefix) + kProjectBucketPrefix + - BucketMetadataCache::NormalizeBucketName(bucket_name), - kGlobalLocation}; - cache->Put(bucket_name, std::move(entry)); + cache->Put(bucket_name, + BucketCacheEntry::FromUnknownProject(bucket_name)); } }); } @@ -324,8 +317,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection { google::cloud::storage_experimental::OTelSpanEnrichmentOption>(); if (!enabled) return; auto entry = BucketCacheEntry::FromLocation( - std::string(kResourceNamePrefix) + bucket.project() + "/buckets/" + - BucketMetadataCache::NormalizeBucketName(bucket_name), + BucketCacheEntry::ResourceName(bucket.project(), bucket_name), bucket.location(), bucket.location_type()); EnrichSpan(span, entry); cache.Put(bucket_name, std::move(entry)); diff --git a/google/cloud/storage/internal/bucket_metadata_cache.cc b/google/cloud/storage/internal/bucket_metadata_cache.cc index 1fa321156ef54..7d7aa312864d8 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache.cc @@ -24,6 +24,13 @@ namespace cloud { namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +std::string BucketCacheEntry::ResourceName(std::string const& project, + std::string const& bucket) { + return std::string(kStorageResourceNamePrefix) + + (project.empty() ? std::string("projects/_") : project) + "/buckets/" + + BucketMetadataCache::NormalizeBucketName(bucket); +} + BucketCacheEntry BucketCacheEntry::FromLocation( std::string id, std::string location, std::string const& location_type) { if (location_type == "multi-region" || location_type == "dual-region") { @@ -34,10 +41,16 @@ BucketCacheEntry BucketCacheEntry::FromLocation( BucketCacheEntry BucketCacheEntry::FromMetadata( storage::BucketMetadata const& m) { - return FromLocation("//storage.googleapis.com/projects/" + - std::to_string(m.project_number()) + "/buckets/" + - m.name(), - m.location(), m.location_type()); + auto project = m.project_number() == 0 + ? std::string{} + : "projects/" + std::to_string(m.project_number()); + return FromLocation(ResourceName(project, m.name()), m.location(), + m.location_type()); +} + +BucketCacheEntry BucketCacheEntry::FromUnknownProject( + std::string const& bucket) { + return {ResourceName(std::string{}, bucket), "global"}; } std::string BucketMetadataCache::NormalizeBucketName( diff --git a/google/cloud/storage/internal/bucket_metadata_cache.h b/google/cloud/storage/internal/bucket_metadata_cache.h index d60a821a55df9..36a8e81c1a26d 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.h +++ b/google/cloud/storage/internal/bucket_metadata_cache.h @@ -38,13 +38,33 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +// The App Hub / Cloud Asset Inventory full resource name prefix for Cloud +// Storage resources. +inline constexpr char kStorageResourceNamePrefix[] = + "//storage.googleapis.com/"; + struct BucketCacheEntry { std::string id; std::string location; + /** + * Returns the full resource name for a bucket, i.e. + * `//storage.googleapis.com/{project}/buckets/{bucket}`. + * + * @param project the project in `projects/{project-id-or-number}` format. If + * empty, `projects/_` is used. + * @param bucket the bucket name, optionally in `projects/_/buckets/{bucket}` + * format. + */ + static std::string ResourceName(std::string const& project, + std::string const& bucket); + static BucketCacheEntry FromLocation(std::string id, std::string location, std::string const& location_type); static BucketCacheEntry FromMetadata(storage::BucketMetadata const& m); + // Creates an entry for a bucket whose project and location are unknown, + // e.g. when fetching its metadata fails with `kPermissionDenied`. + static BucketCacheEntry FromUnknownProject(std::string const& bucket); }; class BucketMetadataCache { diff --git a/google/cloud/storage/internal/bucket_metadata_cache_test.cc b/google/cloud/storage/internal/bucket_metadata_cache_test.cc index b0832cdac74da..7325f6bdcec90 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache_test.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/bucket_metadata_cache.h" +#include "google/cloud/storage/bucket_metadata.h" #include namespace google { @@ -39,6 +40,32 @@ TEST(BucketMetadataCacheTest, NormalizeBucketName) { Eq("test-bucket")); } +TEST(BucketCacheEntryTest, ResourceName) { + EXPECT_THAT(BucketCacheEntry::ResourceName("projects/123", "test-bucket"), + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); + EXPECT_THAT(BucketCacheEntry::ResourceName("projects/123", + "projects/_/buckets/test-bucket"), + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); + EXPECT_THAT(BucketCacheEntry::ResourceName("", "test-bucket"), + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); +} + +TEST(BucketCacheEntryTest, FromMetadata) { + auto entry = BucketCacheEntry::FromMetadata(storage::BucketMetadata() + .set_name("test-bucket") + .set_location("US-EAST1")); + // project_number() defaults to 0, which means "unknown". + EXPECT_THAT(entry.id, + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); +} + +TEST(BucketCacheEntryTest, FromUnknownProject) { + auto entry = BucketCacheEntry::FromUnknownProject("test-bucket"); + EXPECT_THAT(entry.id, + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); + EXPECT_THAT(entry.location, Eq("global")); +} + TEST(BucketMetadataCacheTest, HitAndMiss) { BucketMetadataCache cache(10); EXPECT_FALSE(cache.Get("test-bucket").has_value()); diff --git a/google/cloud/storage/internal/tracing_connection.cc b/google/cloud/storage/internal/tracing_connection.cc index 264d2bf516977..68391cea6028a 100644 --- a/google/cloud/storage/internal/tracing_connection.cc +++ b/google/cloud/storage/internal/tracing_connection.cc @@ -69,20 +69,19 @@ void TracingConnection::MaybeTriggerBackgroundFetch( auto guard = ScopedFetch(cache_, bucket_name); auto current_options = google::cloud::internal::SaveCurrentOptions(); - runner()([impl = impl_, cache = cache_, bucket_name, current_options, - guard]() { - google::cloud::internal::OptionsSpan span(current_options); - storage::internal::GetBucketMetadataRequest request(bucket_name); - auto result = impl->GetBucketMetadata(request); - - if (result.ok()) { - cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); - } else if (result.status().code() == StatusCode::kPermissionDenied) { - cache->Put(bucket_name, - {"//storage.googleapis.com/projects/_/buckets/" + bucket_name, - "global"}); - } - }); + runner()( + [impl = impl_, cache = cache_, bucket_name, current_options, guard]() { + google::cloud::internal::OptionsSpan span(current_options); + storage::internal::GetBucketMetadataRequest request(bucket_name); + auto result = impl->GetBucketMetadata(request); + + if (result.ok()) { + cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); + } else if (result.status().code() == StatusCode::kPermissionDenied) { + cache->Put(bucket_name, + BucketCacheEntry::FromUnknownProject(bucket_name)); + } + }); } void TracingConnection::EnrichSpan(opentelemetry::trace::Span& span,