-
Notifications
You must be signed in to change notification settings - Fork 8
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: add optional InstanceID field to log output #46
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.
Changes are looking good.
I've tried you changes locally and the new feature works as expected.
The only thing I'd like to add is test coverage for the prefix within the log output.
@@ -51,7 +51,7 @@ func TestInitLogger(t *testing.T) { | |||
func TestInitLoggerFile(t *testing.T) { | |||
dir := t.TempDir() | |||
path := dir + "/test.log" | |||
InitLogger(path, DebugLevel.String()) | |||
InitLogger(path, DebugLevel.String(), "test_instance") |
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.
Currently we only test whether the logger is correctly initialized.
Consider adding a test for the prefix as well, e.g.
if !strings.HasPrefix(string(data), "[test_instance]") {
t.Errorf("Expected the output to start with '[test_instance]', but got: %s", string(data))
}
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.
Thanks for the feedback and for testing the code. Will make the change!
Add an optional instanceID field to log output for distinguishing multiple running host-metering instances from each other during development. Signed-off-by: ccowman <[email protected]>
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.
Thanks for adding the test. Changes look good now.
🎉 This PR is included in version 1.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Add an optional instanceID field to log output for distinguishing multiple running host-metering instances from each other during development.
Example implementation of adding an optional, custom instanceID to log output for distinguishing multiple running host-metering instances from each other during development.
The instanceID value is an empty string by default and only added to the log output if the string is populated in the config file.
Any feedback on my approach or suggestions for improvements is appreciated!