Skip to content

test(o11y): add storage grpc tracing integration test#5343

Closed
haphungw wants to merge 4 commits into
googleapis:mainfrom
haphungw:storage-grpc-test
Closed

test(o11y): add storage grpc tracing integration test#5343
haphungw wants to merge 4 commits into
googleapis:mainfrom
haphungw:storage-grpc-test

Conversation

@haphungw
Copy link
Copy Markdown
Contributor

@haphungw haphungw commented Apr 8, 2026

Add an integration test suite for storage gRPC o11y to verify that required attributes are propagated according to OTel semconv.

Also verify that gcp.client.* attributes (such as gcp.client.repo, gcp.client.artifact, gcp.client.version, and gcp.client.service) are correctly included in the InstrumentationScope for metrics.

Note: gcp.client.* attributes are currently missing on the low-level gRPC transport span (T4 tracing) and require to be populated in the transport layer.

@haphungw haphungw force-pushed the storage-grpc-test branch from 2bc45ca to bb8014b Compare April 8, 2026 23:39
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.78%. Comparing base (90667b4) to head (255f0a8).
⚠️ Report is 62 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5343   +/-   ##
=======================================
  Coverage   97.78%   97.78%           
=======================================
  Files         222      222           
  Lines       46128    46128           
=======================================
+ Hits        45104    45107    +3     
+ Misses       1024     1021    -3     

☔ 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.

@haphungw haphungw force-pushed the storage-grpc-test branch 4 times, most recently from 1b9e80e to 6522b8c Compare April 9, 2026 02:20
@haphungw haphungw force-pushed the storage-grpc-test branch from 6522b8c to 77294bd Compare April 9, 2026 02:32
@haphungw haphungw marked this pull request as ready for review April 9, 2026 16:44
@haphungw haphungw requested review from a team as code owners April 9, 2026 16:44
@haphungw
Copy link
Copy Markdown
Contributor Author

haphungw commented Apr 9, 2026

This test was originally started by @westarle. I've picked it up and added the necessary fixes and assertions to verify o11y attributes.

This PR should handle metrics and logging only.

Copy link
Copy Markdown
Contributor

@westarle westarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a lot of cleanup before we submit. If we need it (i.e. we don't have coverage through some other testing) I think it's a good idea to turn it into incremental PRs (i.e. a PR to add the test rig and one test, another PR to add additional tests.)

use tracing_subscriber::util::SubscriberInitExt;

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
pub async fn f1_6_grpc_disablement() -> anyhow::Result<()> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please give these meaningful names that don't depend on the test plan.

.find(|s| s.name == "delete_bucket" || s.name == "google.storage.v2.Storage/DeleteBucket")
.expect("Should have a DeleteBucket span");

/* Temporarily ignore span check
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not ignore the span check?

))
.set_default();

// 1. Setup Mock gRPC Storage Server to fail immediately
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract methods for some of these to reduce reliance on comments...

})
.expect("Should have a DeleteBucket span");

// 5. Verify Metrics
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to break out independent tests, or at least independent verifications.

@haphungw
Copy link
Copy Markdown
Contributor Author

haphungw commented Apr 15, 2026

This PR has been splitted into the following PRs:
#5353
#5355
#5356
#5358
#5359
#5374
#5376
#5377

@haphungw haphungw closed this Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants