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
feat(spanner): include User agent #3465
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -357,3 +357,8 @@ at https://godoc.org/go.opencensus.io/trace. OpenCensus tracing requires Go 1.8 | |
or higher. | ||
*/ | ||
package spanner // import "cloud.google.com/go/spanner" | ||
|
||
// clientUserAgent identifies the version of this package. | ||
// It should be the same as https://pkg.go.dev/cloud.google.com/go/spanner. | ||
// TODO: We will want to automate the version with a bash script. | ||
const clientUserAgent = "spanner-go/v1.12.0" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be dynamic? I am afraid we will forget to update this when a new release comes out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For instance, see 4b1db5f. It would be nice to have a review from @olavloite here too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems we can not do it dynamically in Go. The example you mentioned is in fact define a const, as https://github.com/googleapis/google-cloud-go/blob/master/internal/version/version.go#L29, and https://github.com/grpc/grpc-go/blob/master/version.go#L22. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should really automate this in some way, otherwise it will certainly be forgotten for future releases. The consts that are mentioned above are also automatically generated as part of the release process. The https://github.com/googleapis/google-cloud-go/blob/master/internal/version/version.go#L29 version is generated by this file: https://github.com/googleapis/google-cloud-go/blob/master/internal/version/update_version.sh Could we for now at least add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Bigtable is the only other client that does this today, and recent PRs have shown that the value has been forgotten about over time. If we were going to start doing something like this I think we should have a consistent header key and value that all of our clients use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added a TODO for automating the user agent. There is a plan to use the library version instead of a date, see #2749, so I guess the best solution is to figure out a way to automatically update the library version for all libraries. |
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 think this will be overridden by any
user-agent
that a user has specified in the supplied options here:google-cloud-go/spanner/client.go
Lines 182 to 184 in be88d07
Is that a problem?
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.
This should be good for us. Thanks for catching this!