Update User Agent methods and format #225

shubha-rajan · 2020-06-11T21:11:25Z

Change Description

Updated the SocketFactory classes to set the user agent in CoreSocketFactory so that it is specific to the connector. Also updated the logic in CoreSocketFactory.setApplicationName to append to the existing user agent string if it has already been set, allowing third party tools like spring-cloud-gcp to add custom names.

Checklist

Make sure to open an issue as a
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea.
Ensure the tests and linter pass
Appropriate documentation is updated (if necessary)

Relevant issues:

Fixes Update UserAgent format for usage metrics #219

…ic to append to string

…ctId

shubha-rajan · 2020-06-16T13:14:20Z

Update: currently working for the postgres connector. This will be ready for review once I test the other connectors.

shubha-rajan · 2020-06-16T21:05:46Z

Confirmed that it's working for all connectors (screenshots from Stackdriver Trace)

kurtisvg · 2020-06-17T21:57:59Z

connector-j-5/src/main/java/com/google/cloud/sql/mysql/SocketFactory.java

@@ -31,8 +31,24 @@

  private Socket socket;

+  private static final String APPLICATION_NAME = "mysql-socket-factory-connector-j-5";
+
+  private static String getUserAgentString() {


Rather than run this each time connect is called, can we use a static block so it's only called once? Otherwise this method has to run on each connect.

It would also be convient to try to keep as much of this logic in the core factory as possible, so it doesn't need to be repeated.

static { SocketFactory.setArtifactNameOrSomething("mysql-socket-factory-connector-j-5/" + VERSION ); }

kurtisvg · 2020-06-17T22:06:05Z

connector-j-5/src/main/java/com/google/cloud/sql/mysql/SocketFactory.java

+    try {
+      Properties packageInfo = new Properties();
+      packageInfo
+          .load(SocketFactory.class.getClassLoader().getResourceAsStream("project.properties"));


The gcp-spring folks are getting the artifact version without having to read from a file (example). Can we do something similar here?

Looks like there is a getImplementationtitle that might be helpful as well.

I tried that earlier but kept getting null values for the version. From looking it up, I gathered it was because Maven jar builds don't include that information in the jar manifest by default. I added a config option to the parent pom.xml which added that metadata to the jar, but it still showed up as null when getting the value.

kurtisvg · 2020-06-17T22:15:04Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

@@ -290,7 +296,8 @@ private static SQLAdmin createAdminApiClient(HttpRequestInitializer requestIniti
    JsonFactory jsonFactory = JacksonFactory.getDefaultInstance();
    SQLAdmin.Builder adminApiBuilder =
        new Builder(httpTransport, jsonFactory, requestInitializer)
-            .setApplicationName(getApplicationName());
+            .setApplicationName(
+                String.format("%s %s", getDefaultUserAgent(), getApplicationName()));


What if this is null?

so right now, getDefaultUserAgent defaults to "cloud-sql-java-connector" and getApplicationName defaults to an empty string. So there shouldn't be any cases of either of those returning null.

I'll probably get rid of the getDefaultUserAgent method in the next commit though, since we don't need to set a system property. In that case I'll handle if the connector name is null.

kurtisvg · 2020-06-17T22:15:38Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

@@ -333,6 +340,24 @@ private static KeyPair generateRsaKeyPair() {
    return generator.generateKeyPair();
  }

+  /** Sets the default string which is appended to the SQLAdmin API client User-Agent header. */
+  private static void setDefaultUserAgent(String userAgent) {
+    System.setProperty(APPLICATION_NAME_PROPERTY, userAgent);


I don't think we need to store this in a system property - is only done for getApplicationName() for legacy reasons.

jsimonweb · 2020-06-17T23:17:57Z

For the postgres connector, the /version seems to be getting logged as /null instead of the expected /1.0.17

shubha-rajan · 2020-06-18T01:14:24Z

For the postgres connector, the /version seems to be getting logged as /null instead of the expected /1.0.17

This might have been because of early testing. I tried using another method to get the version and it kept returning null. There should also be some that log the version correctly

jsimonweb · 2020-06-18T17:23:04Z

Got it. LGTM. Logged format postgres-socket-factory/1.0.17-SNAPSHOT

…ctory

kurtisvg

Left a few comments, but apparently didn't attach them to a review correctly (sorry!)

…ties in connectors

shubha-rajan · 2020-06-19T23:28:22Z

Can we use load packageInfo in as a static private field as well?

What happens if the user uses both MySQL and Postgres? Maybe we should maintain a list of UserAgent's that should be used?

I also think we may need to consider to take care with the name - maybe addUserAgent, to avoid conflicting with setApplicationName? thoughts?

I think addUserAgent would make sense if we're maintaining a list of user agents. So then when any of the connectors are loaded into memory, the user agent would be added to the list of user agents. I made some changes to incorporate that in the last commit.

I do have a few questions:

If the user is using both MySQL and Postgres, the user agents are added as soon as the connector classes are loaded because of the static block, right? so then any call to connect would have both user agents, regardless of which connector is making the call. Is this what we want?
Since CoreSocketFactory (and the SQLAdminApi client) is instantiated after the first connect call, is it possible that a user using both MySQL and Postgres would instantiate the SQLAdminAPI before both classes are loaded into memory and both user agents are added?
What's the behavior we want for setApplicationName? This is used by external apps to add their own user agent. Would it make more sense for this string to be stored in a separate variable (so that multiple calls would overwrite it) or should it be appended to the userAgents list?

kurtisvg · 2020-06-22T22:16:01Z

If the user is using both MySQL and Postgres, the user agents are added as soon as the connector classes are loaded because of the static block, right? so then any call to connect would have both user agents, regardless of which connector is making the call. Is this what we want?

Yes - raw number of API calls for each db isn't (IMO) a useful metric, but number of users is.

Since CoreSocketFactory (and the SQLAdminApi client) is instantiated after the first connect call, is it possible that a user using both MySQL and Postgres would instantiate the SQLAdminAPI before both classes are loaded into memory and both user agents are added?

It should always set at least one, but I'm open to suggestions on improving to ensure both are entered. I'm not too terribly worried since this is an edge case.

What's the behavior we want for setApplicationName? This is used by external apps to add their own user agent. Would it make more sense for this string to be stored in a separate variable (so that multiple calls would overwrite it) or should it be appended to the userAgents list?

I think leaving setApplicationName alone for now to preserve compatibility, and just manually appending whatever the property is on client initialization is fine.

kurtisvg · 2020-06-22T22:16:35Z

connector-j-5/src/main/java/com/google/cloud/sql/mysql/SocketFactory.java

+  private static final String DEFAULT_APPLICATION_NAME = "mysql-socket-factory-connector-j-5";
+
+  static {
+    CoreSocketFactory.addUserAgent(DEFAULT_APPLICATION_NAME);


nit: I think at this point you should just inline these.

kurtisvg · 2020-06-22T22:20:36Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

+
+  static {
+    try {
+      Properties packageInfo = new Properties();


Can we move this logic into a function and just call it here so it's a bit more contained? This way we can return "unknown" over null on exception (you may want to check with @jsimonweb if we need to use anything special instead of "unknown")

moved to separate function in latest commit

kurtisvg · 2020-06-22T22:22:01Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

+  /** Sets the default string which is appended to the SQLAdmin API client User-Agent header. */
+  public static void addUserAgent(String artifactId) {
+    if (version != null) {
+      userAgents.add(artifactId + "/" + version);


we should avoid letting version be null and just default it to some safe value (even if empty string). Also, we discussed adding a quick check to avoid repeats.

it's defaulted to "unknown" in the latest commit

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

kurtisvg · 2020-06-22T22:58:55Z

core/src/main/resources/com.google.cloud.sql.core/project.properties

@@ -0,0 +1 @@
+version=${project.version}


nit: should the path just be com.google.cloud.sql since we're using the same version for all of the artifacts?

kurtisvg · 2020-06-22T22:59:31Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

@@ -333,6 +350,28 @@ private static KeyPair generateRsaKeyPair() {
    return generator.generateKeyPair();
  }

+  /** Sets the default string which is appended to the SQLAdmin API client User-Agent header. */
+  public static void addUserAgent(String artifactId) {


nit: Should this be addArtifactId since we're only setting a portion of the intended UserAgent?

kurtisvg · 2020-06-22T22:59:55Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

+  }
+
+  /** Returns the default string which is appended to the SQLAdmin API client User-Agent header. */
+  public static String getUserAgentString() {


Suggested change

public static String getUserAgentString() {

private static String getUserAgents() {

kurtisvg · 2020-06-22T23:00:23Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

+
+  /** Returns the default string which is appended to the SQLAdmin API client User-Agent header. */
+  public static String getUserAgentString() {
+    return String.join(" ", userAgents);


Should this add in USER_TOKEN_PROPERTY_NAME if set?

Not sure. Right now I'm inlining combining the two strings when creating the SQLAdmin API client.

Never mind, changed this to append the application name in the latest set of commits

kurtisvg · 2020-06-23T17:58:12Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

+      Properties packageInfo = new Properties();
+      packageInfo.load(CoreSocketFactory.class.getClassLoader().getResourceAsStream(
+          "com.google.cloud.sql/project.properties"));
+      return packageInfo.getProperty("version");


nit: Should this have a default?

add user agent strings from connectors, update setApplicationName log…

06e1645

…ic to append to string

blunderbuss-gcf bot assigned kurtisvg Jun 11, 2020

shubha-rajan added 5 commits June 15, 2020 11:03

move setApplicationName to constructor in postgres socket factory

bb105e2

pass in user agent string to CoreSocketFactory through props

05905fd

use .properties to set user agent string using POM version and artifa…

6c7d0f5

…ctId

add getter/setter for default user agent

664aa27

update setting applicationName for SQLAdminAPI

732387a

shubha-rajan requested review from kurtisvg and jsimonweb June 16, 2020 21:06

shubha-rajan marked this pull request as ready for review June 16, 2020 21:06

kurtisvg suggested changes Jun 17, 2020

View reviewed changes

moved setting user agent string to static block, remove system property

421be34

shubha-rajan requested a review from kurtisvg June 18, 2020 16:40

jsimonweb approved these changes Jun 18, 2020

View reviewed changes

rename path to project.properties file, get version from CoreSocketFa…

f8c883f

…ctory

kurtisvg suggested changes Jun 19, 2020

View reviewed changes

change userAgent to list of userAgents in core, remove project.proper…

e8010f8

…ties in connectors

kurtisvg suggested changes Jun 22, 2020

View reviewed changes

shubha-rajan added 2 commits June 22, 2020 17:28

inline connector name

223a6ee

check if added user agent is already in list

b677b91

shubha-rajan requested review from kurtisvg and jsimonweb June 23, 2020 00:44

rename getUserAgentString to getUserAgents

680f98e

shubha-rajan added 3 commits June 22, 2020 17:47

make getUserAgents private

88898e4

rename addUserAgent to addArtifactId

d7d5876

append application name in getUserAgents

9e4f4c6

kurtisvg approved these changes Jun 23, 2020

View reviewed changes

add default version in CoreSocketFactory

7847c84

shubha-rajan merged commit 6b67a1a into master Jun 24, 2020

shubha-rajan deleted the update-user-agent branch June 24, 2020 18:52

shubha-rajan mentioned this pull request Aug 21, 2020

Update user agents for r2dbc connectors #254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update User Agent methods and format #225

Update User Agent methods and format #225

shubha-rajan commented Jun 11, 2020

shubha-rajan commented Jun 16, 2020

shubha-rajan commented Jun 16, 2020

kurtisvg Jun 17, 2020

kurtisvg Jun 17, 2020

shubha-rajan Jun 18, 2020

kurtisvg Jun 17, 2020

shubha-rajan Jun 18, 2020

shubha-rajan Jun 18, 2020

kurtisvg Jun 17, 2020

jsimonweb commented Jun 17, 2020

shubha-rajan commented Jun 18, 2020

jsimonweb commented Jun 18, 2020

kurtisvg left a comment

shubha-rajan commented Jun 19, 2020

kurtisvg commented Jun 22, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 22, 2020

kurtisvg Jun 22, 2020

shubha-rajan Jun 23, 2020

shubha-rajan Jun 23, 2020

kurtisvg Jun 23, 2020

shubha-rajan Jun 23, 2020

	public static String getUserAgentString() {
	private static String getUserAgents() {

		@@ -0,0 +1 @@
		version=${project.version}

Update User Agent methods and format #225

Update User Agent methods and format #225

Conversation

shubha-rajan commented Jun 11, 2020

Change Description

Checklist

Relevant issues:

shubha-rajan commented Jun 16, 2020

shubha-rajan commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimonweb commented Jun 17, 2020

shubha-rajan commented Jun 18, 2020

jsimonweb commented Jun 18, 2020

kurtisvg left a comment

Choose a reason for hiding this comment

shubha-rajan commented Jun 19, 2020

kurtisvg commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment