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

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

This change is

adding otel tracing decorator for client_download_file

e6f3f1d

shubham-up-47 requested review froma team ascode owners

July 19, 2025 20:51

shubham-up-47 had a problem deploying to external

July 19, 2025 20:51

— with

GitHub Actions Error

product-auto-labelbot added the api: storageIssues related to the Cloud Storage API. label

Jul 19, 2025

Merge branch 'main' into otel_traces_for_write_object

f250c7c

shubham-up-47 had a problem deploying to external

July 19, 2025 20:56

— with

GitHub Actions Error

fix linting

176c89c

shubham-up-47 had a problem deploying to external

July 20, 2025 07:02

— with

GitHub Actions Error

fix linting

3bfae08

shubham-up-47 temporarily deployed to external

July 20, 2025 07:08

— with

GitHub Actions Inactive

Copy link

codecovbot commentedJul 20, 2025•
edited
Loading

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.

Copy link

ContributorAuthor

shubham-up-47 commentedJul 20, 2025

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

In this, I am logging a newtraceWriteObject/WriteObjectBufferSize from theWriteObjectImpl implementation (along with the existing traces ofWriteObject api).

Does this trace look okay for that feature request?

shubham-up-47 marked this pull request as draft

July 20, 2025 11:22

Copy link

ContributorAuthor

shubham-up-47 commentedJul 24, 2025

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

Including@bajajneha27 too. WDYT?

Copy link

Contributor

bajajneha27 commentedJul 25, 2025

Hi@ddelgrosso1, I raised this PR for the feature request#11394.
In this, I am logging a newtraceWriteObject/WriteObjectBufferSize from theWriteObjectImpl implementation (along with the existing traces ofWriteObject api).
Does this trace look okay for that feature request?
Including@bajajneha27 too. WDYT?

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

Copy link

ContributorAuthor

shubham-up-47 commentedJul 29, 2025•
edited
Loading

Hi@ddelgrosso1, I raised this PR for the feature request#11394.
In this, I am logging a newtraceWriteObject/WriteObjectBufferSize from theWriteObjectImpl implementation (along with the existing traces ofWriteObject api).
Does this trace look okay for that feature request?
Including@bajajneha27 too. WDYT?
Looks like you're only moving the implementation ofWriteObjectBufferSize. Is that intentional?

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

The traces (likeUploadChunk,CreateResumableUpload) which are emitted by theWriteObject 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.

Merge branch 'main' into otel_traces_for_write_object

182d56b

shubham-up-47 had a problem deploying to external

July 31, 2025 07:23

— with

GitHub Actions Error

shubham-up-47 added2 commits

July 31, 2025 08:32

changing approach to generate the traces

d1f6e2c

resolving merge conflicts

af3f6e5

shubham-up-47 had a problem deploying to external

July 31, 2025 09:19

— with

GitHub Actions Error

shubham-up-47 marked this pull request as ready for review

July 31, 2025 10:18

removing extra imports

b6c9932

shubham-up-47 had a problem deploying to external

August 3, 2025 17:19

— with

GitHub Actions Error

Merge branch 'main' into otel_traces_for_write_object

7bd695f

shubham-up-47 temporarily deployed to external

August 3, 2025 17:22

— with

GitHub Actions Inactive

Copy link

ContributorAuthor

shubham-up-47 commentedAug 5, 2025

Hi@ddelgrosso1, I raised this PR for the feature request#11394.
In this, I am logging a newtraceWriteObject/WriteObjectBufferSize from theWriteObjectImpl implementation (along with the existing traces ofWriteObject api).
Does this trace look okay for that feature request?
Including@bajajneha27 too. WDYT?
Looks like you're only moving the implementation ofWriteObjectBufferSize. Is that intentional?
Yes, It is intentional so that we can have a unique trace which is emitted by theWriteObject api call only.
The traces (likeUploadChunk,CreateResumableUpload) which are emitted by theWriteObject 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 theWriteObject/SetupObjectWriteStream trace. This approach looks better to me, please have a look.

shubham-up-47 requested a review frombajajneha27

August 18, 2025 05:57

Copy link

Contributor

bajajneha27 commentedAug 18, 2025

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

shubham-up-47 marked this pull request as draft

August 18, 2025 06:29

Labels

api: storage

Issues related to the Cloud Storage API.

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(storage): Create OTel tracing decorator for`client:: WriteObject()`#15290

Are you sure you want to change the base?

feat(storage): Create OTel tracing decorator for`client:: WriteObject()`#15290

Uh oh!

Conversation

shubham-up-47 commentedJul 19, 2025•
edited by scotthart
Loading

Uh oh!

Uh oh!

codecovbot commentedJul 20, 2025•
edited
Loading

Uh oh!

Codecov Report