-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(storage): Instrument existing metadata ops with storage trace package #11107
chore(storage): Instrument existing metadata ops with storage trace package #11107
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@BrennaEpp could you PTAL and help check the CLA, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start on this!
Co-authored-by: Brenna N Epp <[email protected]>
53726f1
to
f70a8e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor nit, otherwise looks good to me at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Cathy!
opts := []option.ClientOption{ | ||
option.WithTelemetryDisabled(), | ||
} | ||
multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, bucket string, prefix string, client *Client) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a transportClientTest
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd want to use multiTransportTest
here to test trace instrumentation on the transport-agnostic layer. Also, we need to configure NewGRPCClient and NewClient by passing in ClientOption, which cannot be done via transportClientTest
. See previous discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transportClientTest
has a check to not run if the emulator env var is not set
google-cloud-go/storage/client_test.go
Line 2175 in 48022ac
func transportClientTest(ctx context.Context, t *testing.T, test func(*testing.T, context.Context, string, string, storageClient)) { |
We should modify transportClientTest
to allow configuration of client opts similar to multiTransportTest
. Or add a check to this test specifically so it only runs on the emulator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this caused this issue #11480
Update: supersedes #11051