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

Improve handling of canceled calls in logging, metrics and tracing. #180

Merged
merged 7 commits into from
Mar 5, 2020

Conversation

bboreham
Copy link
Collaborator

Replacement for #159, including testing.

The visible improvement is that gRPC streaming functions no longer cause a log message when canceled by the caller.

Canceled functions of all kinds no longer show as an error in tracing.

Also note that canceled gRPC functions will now show up in metrics under the label cancel rather than error.

There are a lot of diffs from the re-generation of a protobuf file, which I separated to its own commit.

Copy link

@thorfour thorfour left a comment

Choose a reason for hiding this comment

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

LGTM


// IsCanceled checks whether an error comes from an operation being canceled
func IsCanceled(err error) bool {
if err == context.Canceled {

Choose a reason for hiding this comment

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

Maybe use errors.Is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would force consumers to upgrade to Go 1.13, I think.
Which is not a bad idea, but more work than I want to complete before merging this.
Do you want to file it as an issue/enhancement?

Choose a reason for hiding this comment

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

That's a good consideration. I can file it as an enhancement

Copy link
Contributor

@tomwilkie tomwilkie left a comment

Choose a reason for hiding this comment

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

LGTM!

@bboreham bboreham merged commit 913d088 into master Mar 5, 2020
@bboreham bboreham deleted the better_cancel_handling branch March 5, 2020 12:34
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.

3 participants