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

Use of log pkg and Fatal in Controller #6

Closed
embano1 opened this issue Jan 24, 2019 · 2 comments
Closed

Use of log pkg and Fatal in Controller #6

embano1 opened this issue Jan 24, 2019 · 2 comments

Comments

@embano1
Copy link
Contributor

embano1 commented Jan 24, 2019

Since the SDK is used as a library and dependency for building connectors, it should leave logging and error handling in the responsibility of the user of the SDK, e.g.

  • make logger configurable (currently hardcoded to "log")
  • avoid log.Fatal calls, e.g. here
  • make extensive use of returning errors to the caller, following the widely used go conventions of error handling in main()
@alexellis
Copy link
Member

Hi Michael,

Thanks for these suggestions.

I'm ok with using log.Fatal in the place of log + panic.

Generally in the OpenFaaS codebase we don't have configurable logging or log-levels.

Which parts of logging would we disable? If you have some specific suggestions we should look at a flag to control that.

I see your points on improving the library - the SDK is meant to be single purposed and used in connectors like the ones we already have. (We should be able to defend all the logging statements or remove them.)

Alex

@embano1
Copy link
Contributor Author

embano1 commented Jan 26, 2019

Thx Alex for your feedback,

Speaking of a SDK, we should be not opinionated about the way we fatal/panic or log. I think it would improve the SDK if logging is configurable and we avoid fatal/panic as much as possible as this gives the user full control of the behavior of the system.

Proposal: add an interface type Logger interface with a minimum set of Print statements (e.g. Printf()). Accept a logger when creating the controller, if logger is nil, default to log.Logger. In case of error, return this to the caller instead of fatal/panic.

Related reading: https://dave.cheney.net/2015/11/05/lets-talk-about-logging

embano1 pushed a commit to embano1/connector-sdk that referenced this issue Nov 30, 2019
As discuss with @alexellis in openfaas#6 the use of log.Fatal is encouraged in
specific cases. However, `GetCredentials` in `types/credentials.go` uses
panic and misses the properly formated log statement.

This PR:

- Replaces `panic` with `log.Fatalln`
- syncs Godeps as `dep status` even on master throws `Gopkg.lock is out
of sync with imports and/or Gopkg.toml`

The PR was tested via invoking `./tester` against the most recent
faas-netes after running `make`.

Signed-off-by: Michael Gasch <mgasch@vmware.com>
embano1 pushed a commit to embano1/connector-sdk that referenced this issue Nov 30, 2019
As discuss with @alexellis in openfaas#6 the use of log.Fatal is encouraged in
specific cases. However, `GetCredentials` in `types/credentials.go` uses
panic and misses the properly formated log statement.

This PR:

- replaces `panic` with `log.Fatalln`
- adds a doc comment for the exported function
- syncs Godeps as `dep status` even on master throws `Gopkg.lock is out
of sync with imports and/or Gopkg.toml`

The PR was tested via invoking `./tester` against the most recent
faas-netes after running `make`.

Signed-off-by: Michael Gasch <mgasch@vmware.com>
@embano1 embano1 closed this as completed Apr 25, 2022
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

No branches or pull requests

2 participants