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

Adding error counters for statd clients #197

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

kwoodson
Copy link
Contributor

@kwoodson kwoodson commented Feb 19, 2020

Adding statsd clients for cosmos, k8s, and azure client libraries.

Fixes #130

pkg/metrics/statsd/azure/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/statsd/cosmosdb/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/statsd/k8s/metrics.go Show resolved Hide resolved
pkg/metrics/statsd/k8s/metrics.go Outdated Show resolved Hide resolved
@kwoodson kwoodson force-pushed the error_counter branch 2 times, most recently from 23e6a1a to 326f7d7 Compare February 20, 2020 00:05
@@ -62,6 +62,12 @@ func (t *tracerRoundTripper) RoundTrip(req *http.Request) (resp *http.Response,
"verb": req.Method,
"path": path,
})
} else {
Copy link
Member

Choose a reason for hiding this comment

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

@mjudeikis convinced me that this function was OK back in the day but I am still doubtful about it.

If t.tr.RoundTrip(req) returns an error, then the defer() runs, then ru, err = strconv.ParseFloat(requestCharge, 64) runs successfully, does that mean that the original error gets overwritten and the end user never sees it?

I think we might need to use a different variable name from err throughout the defer() - and here we need to test the original error to decide whether to increment client.cosmosdb.errors or not.

@kwoodson please can you investigate / fix as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jim-minter @mjudeikis I have renamed the err variable inside of the defer and then I have setup the tests to verify the emits are being called.

Is this more in line with what you wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Im a little bit confused about the code bellow (might be lack of coffee).
I think readability of this function is terrible (my fault).
Maybe we should not do empty return with return t.tr.RoundTrip(req)

and do something like :

defer(){
// RU cound parsing and checking
}
resp, err:= RoundTrip(req)
if err!=nil{
// emit CosmosDB error metric
}
return resp, err

As now this empty return and enourmous defer is confusing a lot!

@kwoodson kwoodson force-pushed the error_counter branch 2 times, most recently from 886c073 to db36761 Compare February 24, 2020 17:34
@kwoodson
Copy link
Contributor Author

Updated to fix import groups in test file.

@kwoodson
Copy link
Contributor Author

Failing on the assert library. Will update.

	imports github.com/stretchr/testify/assert: cannot find package "github.com/stretchr/testify/assert" in any of:

t: time.Now(),
})
tr := New(m)
tr.EndSpan(ctxValue, 401, errors.New("authorizaiton failed"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

@kwoodson
Copy link
Contributor Author

@mjudeikis @jim-minter PTAL, thanks.

Copy link
Member

@jim-minter jim-minter left a comment

Choose a reason for hiding this comment

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

some notes - let's discuss the tricky defer bit on a call.

)

func TestTracer(t *testing.T) {
gc := gomock.NewController(t)
Copy link
Member

Choose a reason for hiding this comment

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

preferred for consistency with the rest of the codebase:

controller := gomock.NewController(t)
defer controller.Finish()

(throughout PR)

pkg/metrics/statsd/azure/metrics_test.go Outdated Show resolved Hide resolved
pkg/metrics/statsd/azure/metrics_test.go Outdated Show resolved Hide resolved
pkg/metrics/statsd/cosmosdb/metrics.go Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

Client error count metric.
4 participants