Skip to content
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

fix(spanner): pass userAgent to cloud spanner requests #6598

Merged
merged 5 commits into from Sep 2, 2022

Conversation

rahul2393
Copy link
Contributor

No description provided.

@rahul2393 rahul2393 requested review from a team as code owners September 1, 2022 10:30
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: spanner Issues related to the Spanner API. labels Sep 1, 2022
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
// UserAgent is the prefix to the user agent header. This is used to supply information
// such as application name or partner tool.
// Recommended format: ``application-or-tool-ID/major.minor.version``.
UserAgent string
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now an exported field that could be used by anyone to set any random value. Would it be possible to either:

  1. Somehow make this unexported? I know that it is called from a different package, so literally making it unexported without any other changes won't work. But would it be possible to in some way or another make this less accessible?
  2. Or otherwise: At least mark this as an internal option that users should not set, and have a fixed list of values that are allowed to be set. Now anyone could set any value, and it does not in any way tell you that users should normally not do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What will be the issue if user set custom values? since this will just append to the existing header
example if userAgent is "go-sql-spanner/0.1" the header will look like

gl-go/1.19.0 go-sql-spanner/0.1 gapic/1.36.0 gax/2.4.0 grpc/1.48.0

So no breaking changes to existing metrics and tracking, from driver and ORMs we will not expose this field for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In python also we are not adding any validation/check.
Example for Django we pass like this: googleapis/python-spanner-django#140
And in the client lib there are no validations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this does not change anything? The handwritten Spanner client will normally also call setClientInfo with gccl/<version>. That seems to be overwritten if the application has set a user agent. At least, in your example above the gccl/<version> part seems to be missing, which means that it is not just appending.

And even then I would suggest that we at least document UserAgent as an internal field. There's nothing that users can do with it, and it might confuse them if they can set it as if it is the same as any other field, but they won't see it turn up anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, please take a look

@hengfengli
Copy link
Contributor

The userAgent will be passed as a ClientOption in: https://github.com/googleapis/go-sql-spanner/blob/6d164d7dc7ac109735dacd1e146d5c16b53b5922/driver.go#L227-L235

The user ClientOption will override the default one fmt.Sprintf("spanner-go/v%s", internal.Version):

func allClientOpts(numChannels int, userOpts ...option.ClientOption) []option.ClientOption {
generatedDefaultOpts := vkit.DefaultClientOptions()
clientDefaultOpts := []option.ClientOption{
option.WithGRPCConnectionPool(numChannels),
option.WithUserAgent(fmt.Sprintf("spanner-go/v%s", internal.Version)),
internaloption.EnableDirectPath(true),
}
allDefaultOpts := append(generatedDefaultOpts, clientDefaultOpts...)
return append(allDefaultOpts, userOpts...)
}

I wonder why this is not working?

@rahul2393
Copy link
Contributor Author

@hengfengli Yes the override is working fine, but x-goog-api-client is not using the ClientOptions, and internally we use the header value for this key for tracking requests.

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 1, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 2, 2022
@rahul2393 rahul2393 merged commit 59d162b into main Sep 2, 2022
@rahul2393 rahul2393 deleted the add-req-useragent branch September 2, 2022 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants