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

What are your thoughts on a package-level logger that is disabled by default? #10

Closed
atc0005 opened this issue Mar 29, 2020 · 1 comment

Comments

@atc0005
Copy link
Collaborator

atc0005 commented Mar 29, 2020

Overview

Continuation of the discussion from #4.

Speaking as someone without a lot of experience writing packages/libraries, it seems like it could free the package to more liberally log useful troubleshooting details, while not emitting it into the usual stdout/stderr streams by default.

I'm testing this functionality from a fork of this project and one of the items I'm emitting is the JSON that is being sent to Teams via the webhook URL. I'm finding that this is useful troubleshooting information as I develop client app changes. I understand that specific JSON logging use-case might not extend to all users of the library, so perhaps wrapping it in some further call might be necessary (were it to be included at all).

Example

// logger is a package logger that can be enabled from client code to allow
// logging output from this package when desired/needed for troubleshooting
var logger *log.Logger

func init() {

	// Disable logging output by default unless client code explicitly
	// requests it
	logger = log.New(os.Stderr, "[goteamsnotify] ", 0)
	logger.SetOutput(ioutil.Discard)

}

// EnableLogging enables logging output from this package. Output is muted by
// default unless explicitly requested (by calling this function).
func EnableLogging() {
	logger.SetFlags(log.Ldate | log.Ltime | log.Lshortfile)
	logger.SetOutput(os.Stderr)
}
@atc0005
Copy link
Collaborator Author

atc0005 commented Apr 4, 2020

@dasrick once we work out #6, are you open to including this functionality?

This is an example of where I feel I did not overuse package-level logging*:

default:
    logger.Println("AddSection: No cases matched, all fields assumed to be at zero-value, skipping section")
    //continue
    // we probably need to return an error here so that client code can
    // handle the situation accordingly
    return fmt.Errorf("all fields found to be at zero-value, skipping section")
}

logger.Println("AddSection: section contains at least one non-zero value, adding section")

mc.Sections = append(mc.Sections, s)

* compared to the verbosity of messages logged elsewhere during the initial development of the extended types ...

atc0005 referenced this issue in atc0005/go-teams-notify Apr 9, 2020
Extend error handling by retrieving and providing the
response string from the remote API as part of the error
message returned to the caller if the status code
indicates an error condition.

Based on testing, the response string contains useful
troubleshooting information which can help identify
missing information in the submitted MessageCard payload.

---

Note: This commit contains example `logger` object calls
to illustrate the proposed changes mentioned in #10. Before
the associated PR would be merged, those `logger` calls
would be removed and implemented as part of a separate
PR specific to #10.

refs #10, #6 (loosely)
atc0005 added a commit to atc0005/go-teams-notify that referenced this issue Apr 18, 2020
This was intended to go in with the other work for
dasrick#6, but I think this assignment
statement was unintentionaly trimmed alongside the
removal of the package-level logger statements referenced
in dasrick#10.

I caught this when performing a test rebase to audit
what remaining changes in my fork haven't been included
or proposed in the parent/upstream project.
atc0005 added a commit to atc0005/go-teams-notify that referenced this issue Apr 18, 2020
This was intended to go in with the other work for
dasrick#6, but I think this assignment
statement was unintentionally trimmed alongside the
removal of the package-level logger statements referenced
in dasrick#10.

I caught this when performing a test rebase to audit
what remaining changes in my fork haven't been included
or proposed in the parent/upstream project.
@atc0005 atc0005 closed this as completed Aug 26, 2020
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

1 participant