Capture Client Guest OS and architecture in JDBC #2561
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2561 +/- ##
============================================
- Coverage 51.12% 51.05% -0.07%
+ Complexity 3940 3925 -15
============================================
Files 147 147
Lines 33456 33468 +12
Branches 5604 5607 +3
============================================
- Hits 17105 17088 -17
- Misses 13963 13971 +8
- Partials 2388 2409 +21 ☔ View full report in Codecov by Sentry. |
| @@ -1251,6 +1266,7 @@ public java.sql.Connection connect(String url, Properties suppliedProperties) th | |||
| "Microsoft JDBC Driver " + SQLJdbcVersion.MAJOR + "." + SQLJdbcVersion.MINOR + "." | |||
| + SQLJdbcVersion.PATCH + "." + SQLJdbcVersion.BUILD + SQLJdbcVersion.RELEASE_EXT | |||
| + " for SQL Server"); | |||
| logClientOSAndArchInfo(); | |||
There was a problem hiding this comment.
is this meant to be used elsewhere? if only used once here then there's no need to create a sep method
|
|
||
| loggerExternal.log(Level.FINE, "Client OS: " + osName + ", Architecture: " + osArch); | ||
| } catch (Exception e) { | ||
| loggerExternal.warning("Unable to capture client OS and architecture information."); |
There was a problem hiding this comment.
should add e.getMessage() for more info
|
|
||
| // If all values are empty, return the default app name | ||
| if (osName.isEmpty() && platform.isEmpty() && osArch.isEmpty()) { | ||
| return DEFAULT_APP_NAME; |
There was a problem hiding this comment.
If osName, platform and osArch are empty, then we can avoid String formatting:
if (!osName.isEmpty() && !platform.isEmpty() && !osArch.isEmpty()) {
return String.format(APP_NAME_TEMPLATE, osName, platform, osArch);
}
return DEFAULT_APP_NAME;
There was a problem hiding this comment.
this should be a static and only done once
| @@ -1028,6 +1054,10 @@ String getClassNameLogging() { | |||
| drLogger.finer("Error registering driver: " + e); | |||
| } | |||
| } | |||
| if (loggerExternal.isLoggable(Level.INFO)) { | |||
There was a problem hiding this comment.
For consistent logging level, please use Level.FINE
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java
Outdated
Show resolved
Hide resolved
| @@ -741,7 +762,7 @@ public final class SQLServerDriver implements java.sql.Driver { | |||
| SQLServerDriverStringProperty.APPLICATION_INTENT.getDefaultValue(), false, | |||
| new String[] {ApplicationIntent.READ_ONLY.toString(), ApplicationIntent.READ_WRITE.toString()}), | |||
| new SQLServerDriverPropertyInfo(SQLServerDriverStringProperty.APPLICATION_NAME.toString(), | |||
| SQLServerDriverStringProperty.APPLICATION_NAME.getDefaultValue(), false, null), | |||
| "", false, null), | |||
There was a problem hiding this comment.
Can we keep this unchanged to default to DEFAULT_APP_NAME
| public void testApplicationName() throws SQLException { | ||
| try (Connection conn = DriverManager.getConnection(connectionString); | ||
| Statement stmt = conn.createStatement(); | ||
| ResultSet rs = stmt.executeQuery("SELECT program_name FROM sys.dm_exec_sessions WHERE session_id = @@SPID")) { |
There was a problem hiding this comment.
Good to have this test which is actually querying the program name
This reverts commit 565ee02.
* Capture Client Guest OS and architecture in JDBC
* Added app name with format Microsoft JDBC - {os}, {platform} - {arch}
* Adding log info and getAppNameWithProperties()
* Updated the application name to be set dynamically
* Added log level as FINE
* Added test case to verify application name
* Updated the SQLServerDriverPropertyInfo with default name
* Added static block for initialization
* Added test case for code coverage
* Updated app Name
Use case: Update the application name to "Microsoft JDBC - {OS}, {Platform} - {architecture}"