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

feat(spanner): include User agent #3465

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

mohanli-ml
Copy link
Contributor

Configure the UserAgent in addition to x-goog-api-client. This will be needed for DirectPath.

@mohanli-ml mohanli-ml requested review from skuruppu and a team as code owners December 15, 2020 03:48
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 15, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Dec 15, 2020

// clientUserAgent identifies the version of this package.
// It should be the same as https://pkg.go.dev/cloud.google.com/go/spanner.
const clientUserAgent = "spanner-go/v1.12.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mohanli-ml mohanli-ml Dec 15, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 TODO here that this needs to be automated with a reference to the bash script mentioned above? This is probably something that we would want to implement for all submodules and integrate in the automated release processes.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mohanli-ml mohanli-ml Dec 17, 2020

Choose a reason for hiding this comment

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


// clientUserAgent identifies the version of this package.
// It should be the same as https://pkg.go.dev/cloud.google.com/go/spanner.
const clientUserAgent = "spanner-go/v1.12.0"
Copy link
Contributor

Choose a reason for hiding this comment

The 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 TODO here that this needs to be automated with a reference to the bash script mentioned above? This is probably something that we would want to implement for all submodules and integrate in the automated release processes.

@@ -177,6 +177,7 @@ func NewClientWithConfig(ctx context.Context, database string, config ClientConf
),
),
option.WithGRPCConnectionPool(config.NumChannels),
option.WithUserAgent(clientUserAgent),
Copy link
Contributor

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:

// opts will take precedence above allOpts, as the values in opts will be
// applied after the values in allOpts.
allOpts = append(allOpts, opts...)

Is that a problem?

Copy link
Contributor Author

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!

@mohanli-ml mohanli-ml added the automerge Merge the pull request once unit tests and other checks pass. label Dec 21, 2020
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 22, 2020
@thiagotnunes thiagotnunes merged commit 4e1ef1b into googleapis:master Dec 22, 2020
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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants