Skip to content

Conversation

@shubham-up-47
Copy link
Contributor

@shubham-up-47 shubham-up-47 commented Jul 19, 2025

Moving some implementation logic of the method WriteObjectImpl from client.cc file to connection_impl.cc file, so that complate tracing of client:: WriteObject can be enabled (#11394).

Trace screenshot: https://screenshot.googleplex.com/3FS8Z5pR67ZNx3i


This change is Reviewable

@shubham-up-47 shubham-up-47 requested review from a team as code owners July 19, 2025 20:51
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jul 19, 2025
@codecov
Copy link

codecov bot commented Jul 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.97%. Comparing base (677354c) to head (af3f6e5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15290      +/-   ##
==========================================
- Coverage   92.98%   92.97%   -0.01%     
==========================================
  Files        2402     2402              
  Lines      217996   218045      +49     
==========================================
+ Hits       202694   202721      +27     
- Misses      15302    15324      +22     

☔ View full report in Codecov by Sentry.
📢 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.

@shubham-up-47
Copy link
Contributor Author

Hi @ddelgrosso1, I raised this PR for the feature request #11394.

In this, I am logging a new trace WriteObject/WriteObjectBufferSize from the WriteObjectImpl implementation (along with the existing traces of WriteObject api).

Does this trace look okay for that feature request?

@shubham-up-47 shubham-up-47 marked this pull request as draft July 20, 2025 11:22
@shubham-up-47
Copy link
Contributor Author

Hi @ddelgrosso1, I raised this PR for the feature request #11394.

In this, I am logging a new trace WriteObject/WriteObjectBufferSize from the WriteObjectImpl implementation (along with the existing traces of WriteObject api).

Does this trace look okay for that feature request?

Including @bajajneha27 too. WDYT?

@bajajneha27
Copy link
Contributor

Hi @ddelgrosso1, I raised this PR for the feature request #11394.
In this, I am logging a new trace WriteObject/WriteObjectBufferSize from the WriteObjectImpl implementation (along with the existing traces of WriteObject api).
Does this trace look okay for that feature request?

Including @bajajneha27 too. WDYT?

Looks like you're only moving the implementation of WriteObjectBufferSize. Is that intentional?

@shubham-up-47
Copy link
Contributor Author

shubham-up-47 commented Jul 29, 2025

Hi @ddelgrosso1, I raised this PR for the feature request #11394.
In this, I am logging a new trace WriteObject/WriteObjectBufferSize from the WriteObjectImpl implementation (along with the existing traces of WriteObject api).
Does this trace look okay for that feature request?

Including @bajajneha27 too. WDYT?

Looks like you're only moving the implementation of WriteObjectBufferSize. Is that intentional?

Yes, It is intentional so that we can have a unique trace which is emitted by the WriteObject api call only.

The traces (like UploadChunk, CreateResumableUpload) which are emitted by the WriteObject api call currently, are emitted in other api calls too, that's why there is no unique otel trace emitted by WriteObject api call.

To have that, i was proposing the trace WriteObjectBufferSize here. If this doesn't look good, we can have another method something like SetupObjectWriteStream or InitializeObjectWriteStreamParams to initialize all the params of the next function call (i.e. ObjectWriteStream), currently i was initializing only one param of the next method call.

@shubham-up-47 shubham-up-47 marked this pull request as ready for review July 31, 2025 10:18
@shubham-up-47
Copy link
Contributor Author

Hi @ddelgrosso1, I raised this PR for the feature request #11394.
In this, I am logging a new trace WriteObject/WriteObjectBufferSize from the WriteObjectImpl implementation (along with the existing traces of WriteObject api).
Does this trace look okay for that feature request?

Including @bajajneha27 too. WDYT?

Looks like you're only moving the implementation of WriteObjectBufferSize. Is that intentional?

Yes, It is intentional so that we can have a unique trace which is emitted by the WriteObject api call only.

The traces (like UploadChunk, CreateResumableUpload) which are emitted by the WriteObject api call currently, are emitted in other api calls too, that's why there is no unique otel trace emitted by WriteObject api call.

To have that, i was proposing the trace WriteObjectBufferSize here. If this doesn't look good, we can have another method something like SetupObjectWriteStream or InitializeObjectWriteStreamParams to initialize all the params of the next function call (i.e. ObjectWriteStream), currently i was initializing only one param of the next method call.

Created a struct ObjectWriteStreamParams and using method SetupObjectWriteStream to initialize all the params of the next function which emits the WriteObject/SetupObjectWriteStream trace. This approach looks better to me, please have a look.

@bajajneha27
Copy link
Contributor

@shubham-up-47 As discussed offline, I think the task is to add a span for the WriteObject as a whole operation which we're missing right now.

@shubham-up-47 shubham-up-47 marked this pull request as draft August 18, 2025 06:29
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.

2 participants