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

Enhance logic to retrieve application name #41

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

RoyZhang2022
Copy link
Contributor

@RoyZhang2022 RoyZhang2022 commented Jul 18, 2023

Issue #, if available:
Enhance logic to retrieve application name(#28)

Description of changes:

  • Get application name by using tasklist /fo csv /nh to get all process info on Windows. Then matching the process ID to get the application name
  • Get application name by using ps -eo pid,comm to get all process info on Linux and macOS. Then matching the process ID to get the application name
  • Fix dependency vulnerability, Bump guava to 32.0.1-jre
  • Update codeql-action to v2 for codeql-analysis workflow
  • Add unit test for get application name.

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

Copy link
Contributor

@alexey-temnikov alexey-temnikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Unit tests are missing
  2. 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(
Copy link
Contributor

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)

Copy link
Contributor Author

@RoyZhang2022 RoyZhang2022 Jul 18, 2023

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.

@sethusrinivasan
Copy link
Contributor

Is there a connection string setting that customers can specify to turn off this capability?

@RoyZhang2022
Copy link
Contributor Author

Is there a connection string setting that customers can specify to turn off this capability?

Currently there is no such a setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants