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

feat: Add config option for log level #332

Merged
merged 1 commit into from Jan 29, 2024
Merged

Conversation

MikeGoldsmith
Copy link
Contributor

Which problem is this PR solving?

Honeytail logs a summary of every batch of events sent to Honeycomb. These are very repetitive and obscure other information if there was a problem sending events. The summaries are logged at the INFO level using logrus.

This PR adds a log level configuration option that allows users to set their desired log level in the logging framework logrus. The usage of info level was verified to not contain critical information if there was a problem, with more important logs using a higher log level (eg error).

Short description of the changes

  • Add new log_level option, defaulting to info
  • If the new option has a value, attempt to set that log level in logrus

This was tested by setting the log_level flag after building the tool locally. As we use flag parsing in main file, it's not easy to write unit tests.

@@ -92,6 +92,8 @@ type GlobalOptions struct {
FilterFiles []string `short:"F" long:"filter-file" description:"Log file(s) to exclude from --file glob. May have multiple values, including multiple globs." yaml:"filter-file,omitempty"`
RenameFields []string `long:"rename_field" description:"Format: 'before=after'. Rename field called 'before' from parsed lines to field name 'after' in Honeycomb events. May have multiple values." yaml:"rename_field,omitempty"`

LogLevel string `long:"log_level" description:"Set the log level. Valid values are 'debug', 'info', 'warn', 'error', 'fatal', 'panic'." default:"info" yaml:"log_level,omitempty"`
Copy link

Choose a reason for hiding this comment

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

Wow, I didn't know logrus supported fatal and panic levels. I wonder if users would find them useful when using honeytail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably not. I let Co-Pilot auto generate the comment here 😄

We have log entries for debug, info, error and fatal. We could remove warn and panic as they are unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming back to this, I think the comment is valid - panic and warn log levels are valid, we just don't use them at the moment.

@MikeGoldsmith MikeGoldsmith merged commit da4beae into main Jan 29, 2024
4 checks passed
@MikeGoldsmith MikeGoldsmith deleted the mike/log-summaries branch January 29, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow disabling logs
2 participants