Added optional request and response logging #6

cihantas · 2018-10-14T07:30:57Z

The user can set a field Debug bool to true upon creation of a helix.Client. An optional custom logger of type log.Logger can also be set. If none is set, a default logger will be used.

Closes #2

…ly provided logger.

coveralls · 2018-10-14T07:39:08Z

Pull Request Test Coverage Report for Build 55

10 of 14 (71.43%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 91.032%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
helix.go	10	14	71.43%

Totals
Change from base Build 53:	-0.5%
Covered Lines:	538
Relevant Lines:	591

💛 - Coveralls

coveralls · 2018-10-14T07:39:08Z

Pull Request Test Coverage Report for Build 58

12 of 26 (46.15%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.0%) to 89.552%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
helix.go	12	26	46.15%

Totals
Change from base Build 53:	-2.0%
Covered Lines:	540
Relevant Lines:	603

💛 - Coveralls

nicklaw5

This looks like a worthwhile contribution. Thanks for taking the time.

Can I suggest we also add both httputil.DumpRequest and httputil.DumpResponse inside the doRequest method? Those functions provide a more detailed account of the request and response.

If possible, can you we also add a test?

Oh! One last thing: we need to add some documentation to the docs page to show an example of how to use the logger.

nicklaw5 · 2018-10-14T08:21:52Z

helix.go

@@ -39,6 +40,10 @@ type Client struct {

 	baseURL      string
 	lastResponse *Response
+
+	// Logging
+	Debug  bool


This field shouldn't be exported (ie. it should have a lowercase d).

This is actually something I want to fix in the future. There's no need to have both the Client and Options structs share the same fields. I'm probably going to add a opts field to the Client struct instead of the individual fields. Leave that to me, however, that's not a necessary for this PR.

nicklaw5 · 2018-10-14T08:22:11Z

helix.go

+
+	// Logging
+	Debug  bool
+	Logger *log.Logger


Same as above: this field shouldn't be exported.

nicklaw5 · 2018-10-14T08:22:42Z

helix.go

@@ -52,6 +57,9 @@ type Options struct {
 	Scopes          []string
 	HTTPClient      HTTPClient
 	RateLimitFunc   RateLimitFunc
+


Drop this empty line.

nicklaw5 · 2018-10-14T08:22:52Z

helix.go

@@ -108,9 +116,18 @@ func NewClient(options *Options) (*Client, error) {
 	c := &Client{
 		clientID:   options.ClientID,
 		httpClient: http.DefaultClient,
+


Drop this empty line.

nicklaw5 · 2018-10-14T08:27:29Z

helix.go

@@ -151,11 +168,19 @@ func (c *Client) sendRequest(method, path string, respData, reqData interface{})
 		return nil, err
 	}

+	if c.Debug == true {


if c.debug { is sufficient here. (Note the lowercase d.)

nicklaw5 · 2018-10-14T08:28:03Z

helix.go

 	err = c.doRequest(req, resp)
 	if err != nil {
 		return nil, err
 	}

+	if c.Debug == true {


Same as above: if c.debug { is sufficient here. (Note the lowercase d.)

cihantas · 2018-10-14T09:59:52Z

This looks like a worthwhile contribution. Thanks for taking the time.

A big thank you back to you for taking the time to write and most importantly publish this library.

Can I suggest we also add both httputil.DumpRequest and httputil.DumpResponse inside the doRequest method? Those functions provide a more detailed account of the request and response.

Sure.

If possible, can you we also add a test?

I'm not very experienced when it comes to testing logging. I did some research and read that most people avoid it because it seems overly excessive. Your guidance on that topic would be welcome.

Oh! One last thing: we need to add some documentation to the docs page to show an example of how to use the logger.

Sure, docs are super important. 👍

nicklaw5

I did some research and read that most people avoid it because it seems overly excessive.

That seems reasonable. I'm happy to forgo tests for this feature.

There is also a list of all Options that can be found here. Do you mind adding the two new fields you've created to that list?

nicklaw5 · 2018-10-14T19:50:13Z

helix.go

@@ -124,7 +124,7 @@ func NewClient(options *Options) (*Client, error) {

 	// Use the default logger, if none was set by the user.
 	if options.Logger == nil {
-		c.Logger = &log.Logger{}
+		c.logger = &log.Logger{}


We also need to set the default output here. If we don't, calling c.logger.Printf will throw an error.

By default I think we should just use stdout. For example:

c.logger = log.New(os.Stdout, "Helix: ", log.LstdFlags)

See the docs for log.New.

nicklaw5 · 2018-10-14T19:53:54Z

docs/logging_docs.md

@@ -0,0 +1,28 @@
+# Logging Documentation


This general documentation doesn't require it's own file. I would prefer we include this at the bottom of the docs README.

nicklaw5 · 2018-10-14T19:57:04Z

docs/logging_docs.md

+## Adding a custom logger
+
+You can add a custom logger to the client that will be used to print outgoing requests and incoming responses. Debug
+mode must be set to `true` to enable logging.


We should also add:

Should you want to enable debug mode but not want to use a custom logger, stdout will be used as the default log output.

nicklaw5 · 2018-10-14T19:59:07Z

docs/logging_docs.md

+// Create a logger, which outputs to file.
+f, err := os.OpenFile("log.txt", os.O_RDWR | os.O_CREATE | os.O_APPEND, 0666)
+if err != nil {
+    log.Fatalf("error opening file: %v", err)


Let's replace this line with // handle error to be consistent with rest of the documentation.

nicklaw5 · 2018-10-14T20:01:02Z

docs/README.md

@@ -10,6 +10,7 @@ Follow the links below to their respective API usage examples:
 - [Clips](clips_docs.md)
 - [Entitlement Grants](entitlement_grants_docs.md)
 - [Games](games_docs.md)
+- [Logging](logging_docs.md)


Given that we're moving the logging docs to this same file, this line can be dropped.

nicklaw5

Just a few minor comments regarding the example. Once those are fixed I think we'll be good to get this merged.

nicklaw5 · 2018-10-15T19:01:09Z

docs/README.md

+// Create a logger, which outputs to file.
+f, err := os.OpenFile("log.txt", os.O_RDWR | os.O_CREATE | os.O_APPEND, 0666)
+if err != nil {
+    log.Fatalf("error opening file: %v", err)


This line should be // handle error

nicklaw5 · 2018-10-15T19:01:24Z

docs/README.md

+    debug: true,	
+})
+if err != nil {
+    log.Fatalf("error opening file: %v", err)


This line should be // handle error

nicklaw5 · 2018-10-15T19:03:24Z

docs/README.md

+fileLogger.SetOutput(f)
+
+client, err := helix.NewClient(&helix.Options{
+    logger: fileLogger,	


logger needs to be capitalised (ie. Logger).

nicklaw5 · 2018-10-15T19:03:39Z

docs/README.md

+
+client, err := helix.NewClient(&helix.Options{
+    logger: fileLogger,	
+    debug: true,	


debug needs to be capitalised (ie. Debug).

nicklaw5 · 2018-12-30T20:21:54Z

@cihantas How you going with this? Did you need any assistance? I'd Iike to get this merged if possible.

nicklaw5 · 2019-10-14T17:06:34Z

Closing. Please re-open if you would like these change reconsidered.

Added request and response logging with a debug field and an optional…

f230abf

…ly provided logger.

cihantas mentioned this pull request Oct 14, 2018

Add an option for logging #2

Open

nicklaw5 requested changes Oct 14, 2018

View reviewed changes

cihantas added 4 commits October 14, 2018 11:37

Fields shouldn't be exported.

6aeff66

Remove newlines.

0133e40

Additionally use httputil.DumpRequest/Response in debug mode.

29e8936

Added logging docs.

b2cddbf

Fix: use non-exported logger field.

273fe2c

nicklaw5 assigned cihantas Oct 14, 2018

nicklaw5 added the enhancement New feature or request label Oct 14, 2018

nicklaw5 requested changes Oct 14, 2018

View reviewed changes

cihantas added 3 commits October 15, 2018 06:43

Remove separate doc file.

a1983d9

Add logging documentation to the bottom of the README.

c51e28b

Use default logger with stdout and Helix prefix.

1b1904c

nicklaw5 requested changes Oct 15, 2018

View reviewed changes

nicklaw5 closed this Oct 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added optional request and response logging #6

Added optional request and response logging #6

cihantas commented Oct 14, 2018 •

edited by nicklaw5

Loading

coveralls commented Oct 14, 2018

coveralls commented Oct 14, 2018 •

edited

Loading

nicklaw5 left a comment •

edited

Loading

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

cihantas commented Oct 14, 2018

nicklaw5 left a comment •

edited

Loading

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018 •

edited

Loading

nicklaw5 Oct 14, 2018

nicklaw5 Oct 14, 2018

nicklaw5 left a comment

nicklaw5 Oct 15, 2018

nicklaw5 Oct 15, 2018

nicklaw5 Oct 15, 2018

nicklaw5 Oct 15, 2018

nicklaw5 commented Dec 30, 2018

nicklaw5 commented Oct 14, 2019

Added optional request and response logging #6

Added optional request and response logging #6

Conversation

cihantas commented Oct 14, 2018 • edited by nicklaw5 Loading

coveralls commented Oct 14, 2018

Pull Request Test Coverage Report for Build 55

💛 - Coveralls

coveralls commented Oct 14, 2018 • edited Loading

Pull Request Test Coverage Report for Build 58

💛 - Coveralls

nicklaw5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cihantas commented Oct 14, 2018

nicklaw5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklaw5 Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklaw5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklaw5 commented Dec 30, 2018

nicklaw5 commented Oct 14, 2019

cihantas commented Oct 14, 2018 •

edited by nicklaw5

Loading

coveralls commented Oct 14, 2018 •

edited

Loading

nicklaw5 left a comment •

edited

Loading

nicklaw5 left a comment •

edited

Loading

nicklaw5 Oct 14, 2018 •

edited

Loading