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

Log Stackdriver request when it fails. #1824

Merged
merged 1 commit into from
Oct 2, 2017
Merged

Conversation

x13n
Copy link
Contributor

@x13n x13n commented Sep 29, 2017

More data for debugging.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2017
@x13n x13n requested a review from piosz October 2, 2017 08:42
@@ -309,6 +310,12 @@ func (sink *StackdriverSink) sendOneRequest(req *sd_api.CreateTimeSeriesRequest)
var responseCode int
if err != nil {
glog.Warningf("Error while sending request to Stackdriver %v", err)
reqJson, errJson := json.Marshal(req)
if errJson != nil {
glog.Warningf("The request was %s", reqJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(2)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please print all requests as V(10)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 2, 2017
Copy link
Contributor

@piosz piosz left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nits

func (sink *StackdriverSink) sendOneRequest(req *sd_api.CreateTimeSeriesRequest) {
startTime := time.Now()
empty, err := sink.stackdriverClient.Projects.TimeSeries.Create(fullProjectName(sink.project), req).Do()

var responseCode int
if err != nil {
glog.Warningf("Error while sending request to Stackdriver %v", err)
if glog.V(2) {
marshalRequestAndLog(func(reqJson []byte) {
glog.Warningf("The request was: %s", reqJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

glog.Infof.V(2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no such syntax. There is glog.V(2).Infof, however. Quoting from glog doc, those two calls are (almost) equivalent:

if glog.V(2) { glog.Info("log this") }

glog.V(2).Info("log this")

The only difference is, when V(2) is false, the expression inside won't get evaluated. I'm using the former, since instead of simply logging a string I'm also lazy-evaluating conversion to json.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was not clear enough. When you are reading the code IMO it's easier to understand:

if glog.V(2) {
  glog.V(2).Info("log this") }
}

than

if glog.V(2) {
  glog.Warninf("log this") }
}

because in the second case your first impression is that the log severity is Warning, but then when you look closer, it turns out the this is in fact V(2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, changed.

func (sink *StackdriverSink) sendOneRequest(req *sd_api.CreateTimeSeriesRequest) {
startTime := time.Now()
empty, err := sink.stackdriverClient.Projects.TimeSeries.Create(fullProjectName(sink.project), req).Do()

var responseCode int
if err != nil {
glog.Warningf("Error while sending request to Stackdriver %v", err)
if glog.V(2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment explaining this if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

switch reflect.Indirect(reflect.ValueOf(err)).Type() {
case reflect.Indirect(reflect.ValueOf(&googleapi.Error{})).Type():
responseCode = err.(*googleapi.Error).Code
default:
responseCode = httpResponseCodeUnknown
}
} else {
if glog.V(10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

switch reflect.Indirect(reflect.ValueOf(err)).Type() {
case reflect.Indirect(reflect.ValueOf(&googleapi.Error{})).Type():
responseCode = err.(*googleapi.Error).Code
default:
responseCode = httpResponseCodeUnknown
}
} else {
if glog.V(10) {
marshalRequestAndLog(func(reqJson []byte) {
glog.Infof("Stackdriver request sent: %s", reqJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

glog.Infof.V(10)

@piosz
Copy link
Contributor

piosz commented Oct 2, 2017

cc @loburm

@x13n x13n force-pushed the master branch 2 times, most recently from 62537e7 to e48452a Compare October 2, 2017 12:39
Failed requests logged at log level 2.
Successful ones logged at log level 10.
@piosz
Copy link
Contributor

piosz commented Oct 2, 2017

/lgtm
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2017
@piosz piosz merged commit ae62e1f into kubernetes-retired:master Oct 2, 2017
@piosz
Copy link
Contributor

piosz commented Oct 2, 2017

@x13n could you please cherry-pick to release-1.4?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants