feat(storage): add resource span attributes for ACO ( App Centric Observability )#16119
feat(storage): add resource span attributes for ACO ( App Centric Observability )#16119bajajneha27 wants to merge 23 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 private helper method EnrichSpan to populate OpenTelemetry span attributes (gcp.resource.destination.id and gcp.resource.destination.location) using bucket metadata upon successful bucket operations (such as creation, retrieval, updates, and locking). It also adds corresponding unit tests to verify these attributes. The review comments suggest making EnrichSpan static since it does not access member variables, and checking for an uninitialized project number (value 0) to avoid generating invalid resource IDs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16119 +/- ##
========================================
Coverage 92.26% 92.27%
========================================
Files 2214 2217 +3
Lines 206307 206837 +530
========================================
+ Hits 190358 190862 +504
- Misses 15949 15975 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bajajneha27
force-pushed
the
509338299
branch
3 times, most recently
from
b9d9575 to
53127d4
Compare
bajajneha27
force-pushed
the
509338299
branch
2 times, most recently
from
c978382 to
ab9f016
Compare
| TracingConnection::TracingConnection(std::shared_ptr<StorageConnection> impl) | ||
| : impl_(std::move(impl)) {} | ||
| : impl_(std::move(impl)), | ||
| background_threads_( |
There was a problem hiding this comment.
This appears to always create an instance of BackgroundThreads. If the user has set the GrpcBackgroundThreadsFactoryOption they would expect the value of that Option to be used without creating another BackgroundThreads internally.
| @@ -31,8 +36,17 @@ google::cloud::storage::Client MakeGrpcClient(Options opts) { | |||
| opts = google::cloud::storage_internal::DefaultOptionsGrpc(std::move(opts)); | |||
| storage_internal::EnableGrpcMetrics(opts); | |||
| auto stub = std::make_unique<storage_internal::GrpcStub>(opts); | |||
There was a problem hiding this comment.
GrpcStub also calls MakeBackgroundThreadsFactory(opts)() internally, which means we are now creating two separate thread pools (and two separate completion queues) for every client. Is there a way to ensure the single pool is cleanly shared?
| if (!runner_) { | ||
| auto threads = | ||
| std::make_shared<google::cloud::rest_internal:: | ||
| AutomaticallyCreatedRestPureBackgroundThreads>(1U); |
There was a problem hiding this comment.
This instantly spins up a dedicated background OS thread the moment the client is created. Should we initialize this lazily only when required?
4 participants
No description provided.