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

Adds a NopLogger to the backend/log package. #709

Closed
wants to merge 1 commit into from

Conversation

joeblubaugh
Copy link

This is useful in testing and occasionally in other scenarios for plugin authors.

What this PR does / why we need it:
Adds a NopLogger to the backend/log package. This is useful in testing and occasionally in other scenarios for plugin authors.

Which issue(s) this PR fixes:

Fixes #708

@joeblubaugh joeblubaugh requested a review from a team as a code owner June 28, 2023 06:58
@joeblubaugh joeblubaugh requested review from andresmgot and xnyo and removed request for a team June 28, 2023 06:58
@CLAassistant
Copy link

CLAassistant commented Jun 28, 2023

CLA assistant check
All committers have signed the CLA.

@joeblubaugh
Copy link
Author

This is a speculative PR. If #708 isn't desired, we can just close.

This is useful in testing and occasionally in other scenarios for plugin authors.
@andresmgot
Copy link
Contributor

This is useful in testing and occasionally in other scenarios for plugin authors.

Can you elaborate the use case when it's useful? For example, in tests, it will only print logs if the test fail, which can be helpful to identify the root cause of the error.

@joeblubaugh
Copy link
Author

Fair point about the tests; I hadn't considered that.

In my case, I implemented this NopLogger locally because I have some code that does complex data transformations over several hundred lines and many functions. In one context, failures are errors and need to be logged. I want to use the logger to ensure that the issue is logged where it occurs and the logs can be monitored. In another case, the errors don't need to be logged, because the transformation is used in a validation step. These errors should not be logged, but errors are returned to the user in a "bad request" form.

@andresmgot
Copy link
Contributor

I want to use the logger to ensure that the issue is logged where it occurs and the logs can be monitored

From this, it seems that you may want to change the logger implementation to assert things on execution? In any case that's not something you can do with this NopLogger.

Since we are using the Hashicorp logger under the hoods, I think that if we add the Off level that they support, it should cover the use case that you want, suppressing any logging for a certain test. Is that correct?

@joeblubaugh
Copy link
Author

Yes, I think adding support for Off would cover my use case.

I don't want to assert in the logger - just pass a different implementation to functions in some cases.

@wbrowne
Copy link
Member

wbrowne commented Feb 28, 2024

Closing this due to inactivity. However, please feel free to continue the conversation here if this is still relevant, and we can re-open if necessary. Thank you!

@wbrowne wbrowne closed this Feb 28, 2024
@joeblubaugh
Copy link
Author

Sorry, I think I took this message as a handoff rather than a request for me to do it:

Since we are using the Hashicorp logger under the hoods, I think that if we add the Off level that they support, it should cover the use case that you want, suppressing any logging for a certain test. Is that correct?

@joeblubaugh
Copy link
Author

This was addressed in a very similar way in #876, so no need to reopen.

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.

Add a No-op logger to the log package
4 participants