-
Notifications
You must be signed in to change notification settings - Fork 9
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
Enhance logic to retrieve application name #41
base: main
Are you sure you want to change the base?
Enhance logic to retrieve application name #41
Conversation
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.
- Unit tests are missing
- Need to add a section about how the change was tested is missing (including a list of scenarios tested, platforms, and expected and actual results).
} | ||
} catch (Exception err) { | ||
// Eat the exception and fall through. | ||
LOGGER.warning( |
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.
If we don't expect it to fail, it would have been best to use error-level logging and propagate the error properly, so it is possible to retrieve exceptions from log files later (that is an essential information for troubleshooting)
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 is not a fatal error that would block the driver running, so warning is a proper log level as it is not serious if application name could not be got.
jdbc/src/main/java/software/amazon/timestream/jdbc/TimestreamDriver.java
Show resolved
Hide resolved
jdbc/src/main/java/software/amazon/timestream/jdbc/TimestreamDriver.java
Outdated
Show resolved
Hide resolved
Is there a connection string setting that customers can specify to turn off this capability? |
Currently there is no such a setting. |
Issue #, if available:
Enhance logic to retrieve application name(#28)
Description of changes:
tasklist /fo csv /nh
to get all process info on Windows. Then matching the process ID to get the application nameps -eo pid,comm
to get all process info on Linux and macOS. Then matching the process ID to get the application name32.0.1-jre
codeql-action
to v2 for codeql-analysis workflowTest
Run unit test on Windows, Linux and macOS to check the application names fetched with expected values, all tests passed.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.