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

Allow disabling stack traces, add test. #192

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

tommyjcarpenter
Copy link
Contributor

@tommyjcarpenter tommyjcarpenter commented Apr 2, 2024

This adds the optional ability to turn off stack traces. It preserves current behavior of including them.

This option is available to users that are instantiating a logger themselves, but its not currently exposed to users of InitLoggerX

An alternative would have been to not do lgrCfg := zap.NewProductionConfig() inside the function and instead take a zap config, but thats not the way the original code was written.

loggingx/config.go Outdated Show resolved Hide resolved
Signed-off-by: Tommy Carpenter <[email protected]>
Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

We would want to ensure existing service behavior is not changed. Some small changes could allow that to happen.

Additionally, more information about the reason for your changes in your pr would be appreciated.

loggingx/logger.go Outdated Show resolved Hide resolved
Signed-off-by: Tommy Carpenter <[email protected]>
loggingx/logger_test.go Outdated Show resolved Hide resolved
@tommyjcarpenter tommyjcarpenter requested a review from mikemrm April 2, 2024 13:59
Signed-off-by: Tommy Carpenter <[email protected]>
mikemrm
mikemrm previously approved these changes Apr 2, 2024
Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

Looks good

Signed-off-by: Tommy Carpenter <[email protected]>
sfunkhouser
sfunkhouser previously approved these changes Apr 2, 2024
@sfunkhouser sfunkhouser disabled auto-merge April 2, 2024 14:37
Copy link
Member

@nicolerenee nicolerenee left a comment

Choose a reason for hiding this comment

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

Minor tweak to names to match the existing convention in the repo requested but otherwise looks good. Thanks for the PR!

loggingx/config.go Outdated Show resolved Hide resolved
loggingx/config.go Outdated Show resolved Hide resolved
loggingx/config.go Outdated Show resolved Hide resolved
Signed-off-by: Tommy Carpenter <[email protected]>
Copy link
Member

@nicolerenee nicolerenee left a comment

Choose a reason for hiding this comment

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

🎉 Thanks for the contribution

@nicolerenee nicolerenee added this pull request to the merge queue Apr 2, 2024
Merged via the queue into infratographer:main with commit 30872a1 Apr 2, 2024
6 checks passed
@tommyjcarpenter tommyjcarpenter deleted the stacktrace branch April 2, 2024 14:57
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.

4 participants