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

Confusion around StructuredLogger #587

Closed
zlesnr opened this issue Dec 30, 2020 · 2 comments
Closed

Confusion around StructuredLogger #587

zlesnr opened this issue Dec 30, 2020 · 2 comments
Labels
stale Issue or PR has been inactive for a period of time. This label is configured in stale-bot.

Comments

@zlesnr
Copy link
Contributor

zlesnr commented Dec 30, 2020

In debugging an issue where the newrelic-cli didn't appear to be logging at the correct log level, I arrived at the SetLevel call in StructuredLogger.

https://github.com/newrelic/newrelic-client-go/blob/master/internal/logging/structured_logger.go#L34

In many client packages, we call config.GetLogger() which will initialize the logger if it has not already been called, and then use the returned logger as the object that we make the log message calls.

https://github.com/newrelic/newrelic-client-go/blob/master/pkg/config/config.go#L94

Related to #576, StructuredLogger is not using a private logger, but configuring the global logger.

This is confusing, because when the client is used from an external package, as is common, it means that the first API call to the API will likely not have reached the GetLogger yet, and so the global logger gets reconfigured. This is not desirable as it means that if the logger was configured outside before an API call is made, then the logger is then modified.

It appears that the primary use of the StructuredLogger is to wrap the WithFields calls on logrus.Logger, but those calls are not accessing any private logger, and so those could be raw methods on the logging package, rather than hanging off the StructuredLogger.

If the goal of the StrcuturedLogger is to use a private logger, #586 will do this, though it may have other unintended consequences. In any case, the private logger is not happening currently.

If instead, we want the client to use the global logger, which it will share with all the callers that depend on this project, then I think we have a little work to do. In this case, it might be advisable to remove the StructuredLogger entirely and instead replace all the config.GetLogger() and the subsequent logger.Info() etc usage with just direct calls to log or move the helpers to public methods on the logging package. For the remaining methods on the StructuredLogger, I think we can discuss the feature deficit, like JSON logging etc, and decide if that is valuable.

Given that this project is a component of other projects, my expectation here is that we have very slim opinions about how logging should be handled, which would allow the caller projects to contain these opinions. Let's discuss which direction we want to take this and move forward.

@stale
Copy link

stale bot commented Jan 14, 2021

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Issue or PR has been inactive for a period of time. This label is configured in stale-bot. label Jan 14, 2021
@zlesnr
Copy link
Contributor Author

zlesnr commented Jan 14, 2021

This should be resolved with #586

@zlesnr zlesnr closed this as completed Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue or PR has been inactive for a period of time. This label is configured in stale-bot.
Projects
None yet
Development

No branches or pull requests

1 participant