feat(storage): add resource span attributes for ACO ( App Centric Observability ) for rewrite API in async client - #16285
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for span enrichment in RewriteObject within the asynchronous connection tracing implementation. It introduces an overload of MakeTracingAsyncRewriterConnection that accepts an OpenTelemetry span, allowing the span to be created and enriched with destination bucket details before being passed to the tracing connection. A unit test has been added to verify that the span is correctly enriched using cached bucket metadata. I have no feedback to provide as the changes are well-implemented and include appropriate test coverage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16285 +/- ##
==========================================
- Coverage 92.31% 92.31% -0.01%
==========================================
Files 2223 2223
Lines 208086 208121 +35
==========================================
+ Hits 192086 192117 +31
- Misses 16000 16004 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gcbrun |
kalragauri
left a comment
There was a problem hiding this comment.
There is a limitation in our overall ACO caching design. Because we call EnrichSpan() immediately at span creation across the async client, a background cache miss results in the span never being enriched. For long-running operations, the cache populates milliseconds later, but the span stays unenriched. In the Java SDK, they avoid this by wrapping spans in an AcoSpan decorator that lazily pulls from the cache right at span.end(). Let's track this as a separate issue.
| auto r = f.get(); | ||
| internal::DetachOTelContext(oc); | ||
| if (!r) { | ||
| span->AddEvent("gl-cpp.storage.rewrite.iterate", |
There was a problem hiding this comment.
There is a pre-existing issue here that we should probably track as a follow-up.
Currently, in AsyncRewriterTracingConnection::Iterate() (and similarly for .Write() and .Read() in the async client), we call span->AddEvent(...) for every single loop iteration.
The risk here is that OpenTelemetry SDKs enforce a limit on the number of events per span. For large object rewrites, .Iterate() can execute hundreds or thousands of times. Once we exceed the limit, OTel silently drops the rest, affecting observability for the tail end of any large data transfer.
Other Storage SDKs have managed to avoid this issue, so let's chat about how C++ can also avoid it.
…ervability ) for rewrite API in async client
ae615ff to
73a7ed6
Compare
kalragauri
left a comment
There was a problem hiding this comment.
nit: pls fix the PR title + description.
Ack! I've created issues to fix both the concerns. |
|
/gcbrun |
No description provided.