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

ZOOKEEPER-3439: Observability improvements on client / server connection close. #997

Conversation

hanm
Copy link
Contributor

@hanm hanm commented Jun 22, 2019

Currently when server closes a client connection there is not enough information recorded (except few exception logs) which makes it hard to do postmortems. On the other side, having a complete view of the aggregated connection closing reason will provide more signals based on which we can better operate the clusters (e.g. predicate an incident might happen based on the trending of the connection closing reasons).

Server metrics was not added in this PR as we internally use a different metrics system - so some work needed to migrate to ServerMetrics. Want to submit first to get community feedback on this.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Awesome.
I feel existing test cases are covering your change.

LGTM

@enixon
Copy link

enixon commented Jun 24, 2019

I like it. :)

@maoling
Copy link
Member

maoling commented Jun 25, 2019

very useful to diagnose the close issue.

@hanm
Copy link
Contributor Author

hanm commented Jul 2, 2019

@eolivelli could you please help commit this now it's approved?

@eolivelli
Copy link
Contributor

@hanm I am out for vacation this week.
I will commit as soon as I have my laptop with me (next Monday)
Btw you already have enough +1 here, I feel you can commit it by yourself, I will be listed as reviewer.

@asfgit asfgit closed this in 9bee98b Jul 2, 2019
@hanm
Copy link
Contributor Author

hanm commented Jul 2, 2019

@eolivelli no worries, just merged it. I usually prefer others to commit my own patch.

@eolivelli
Copy link
Contributor

@hanm I agree with you. It is better to not self merge. You had my explicit consent this time.
Thanks

stickyhipp pushed a commit to stickyhipp/zookeeper that referenced this pull request Aug 2, 2019
…ion close.

Currently when server closes a client connection there is not enough information recorded (except few exception logs) which makes it hard to do postmortems. On the other side, having a complete view of the aggregated connection closing reason will provide more signals based on which we can better operate the clusters (e.g. predicate an incident might happen based on the trending of the connection closing reasons).

Server metrics was not added in this PR as we internally use a different metrics system - so some work needed to migrate to ServerMetrics. Want to submit first to get community feedback on this.

Author: Michael Han <lhan@twitter.com>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, maoling <maoling199210191@sina.com>, enixon

Closes apache#997 from hanm/twitter/cc6d87505d9745caace2c86acf58d6ffa085281e
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Aug 31, 2022
…ion close.

Currently when server closes a client connection there is not enough information recorded (except few exception logs) which makes it hard to do postmortems. On the other side, having a complete view of the aggregated connection closing reason will provide more signals based on which we can better operate the clusters (e.g. predicate an incident might happen based on the trending of the connection closing reasons).

Server metrics was not added in this PR as we internally use a different metrics system - so some work needed to migrate to ServerMetrics. Want to submit first to get community feedback on this.

Author: Michael Han <lhan@twitter.com>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, maoling <maoling199210191@sina.com>, enixon

Closes apache#997 from hanm/twitter/cc6d87505d9745caace2c86acf58d6ffa085281e
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
…ion close.

Currently when server closes a client connection there is not enough information recorded (except few exception logs) which makes it hard to do postmortems. On the other side, having a complete view of the aggregated connection closing reason will provide more signals based on which we can better operate the clusters (e.g. predicate an incident might happen based on the trending of the connection closing reasons).

Server metrics was not added in this PR as we internally use a different metrics system - so some work needed to migrate to ServerMetrics. Want to submit first to get community feedback on this.

Author: Michael Han <lhan@twitter.com>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, maoling <maoling199210191@sina.com>, enixon

Closes apache#997 from hanm/twitter/cc6d87505d9745caace2c86acf58d6ffa085281e
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.

4 participants