Skip to content
This repository has been archived by the owner on Dec 8, 2023. It is now read-only.

Logging whole response in debug mode #101

Merged
merged 1 commit into from
Oct 16, 2017
Merged

Logging whole response in debug mode #101

merged 1 commit into from
Oct 16, 2017

Conversation

mathieucarbou
Copy link
Contributor

@mathieucarbou mathieucarbou commented Oct 12, 2017

Because some other errors might happen than the one handled (stoplist). Example, if the tag values have an invalid format, the response is something like:

{"measurements":{"summary":{"total":174,"accepted":78,"failed":96,"filtered":0}},"errors":[{"param":"tag_value","value":"41454@127_0_0_1","reason":"Invalid format"}]}

So it would be nice to log the whole response in debug mode to avoid discarding important failures.

Because some other errors might happen than the one handled (stoplist). Example, if the tag values have an invalid format, the response is something like:

`{"measurements":{"summary":{"total":174,"accepted":78,"failed":96,"filtered":0}},"errors":[{"param":"tag_value","value":"41454@127_0_0_1","reason":"Invalid format"}]}`

So it would be nice to log the whole response in debug mode to avoid discarding important failures.
Copy link
Contributor

@bryanmikaelian bryanmikaelian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this but we may want to do this for 2xx responses only. 4xx and 5xx will be handled in the lines below but, with the current implementation, the logs might get super chatty. I think you'll get a log statement for the response and an additional log statement for a 4xx/5xx response

cc @jderrett

@mathieucarbou
Copy link
Contributor Author

mathieucarbou commented Oct 12, 2017

@bryanmikaelian : yes it's chatty, and I think this is what you want when in debug mode: this is the only way I have as a user of this plugin to make sure Librato receives the response and processes it and then to see its answer.
Hence the (if(logAll))

@bryanmikaelian
Copy link
Contributor

Cool. Makes sense. I'll see if anyone else on the team has any thoughts and then we can merge / release.

@bryanmikaelian bryanmikaelian added this to the 2.x.x: Tagged Measurements milestone Oct 13, 2017
@mheffner
Copy link
Contributor

If we are logging the request, seems like logging all responses is equally verbose. 👍

@bryanmikaelian
Copy link
Contributor

👍

Thanks for the contribution @mathieucarbou! I'll get this merged and published to npm today.

Copy link
Contributor

@jderrett jderrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bryanmikaelian bryanmikaelian merged commit 53d379c into librato:master Oct 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants