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

fix: stream related metrics #3474

Merged
merged 3 commits into from Nov 1, 2022
Merged

fix: stream related metrics #3474

merged 3 commits into from Nov 1, 2022

Conversation

notanatol
Copy link
Contributor

@notanatol notanatol commented Oct 31, 2022

Checklist

  • I have read the coding guide.
  • I have filled out the description and linked the related issues.

Description

We need metrics to see how many streams we are closing/resetting and how many closed streams libp2p library reports.

Motivation and Context (Optional)

It's likely the libp2p library does not releases the resources after a stream is closed.

Related Issue (Optional)

Closes #3439


This change is Reviewable

@notanatol notanatol added the ready for review The PR is ready to be reviewed label Oct 31, 2022
@notanatol notanatol self-assigned this Oct 31, 2022
@@ -543,7 +543,9 @@ func TestConnectRepeatHandshake(t *testing.T) {
t.Fatal(err)
}

if _, err := s2.HandshakeService().Handshake(ctx, libp2p.NewStream(stream), info.Addrs[0], info.ID); err != nil {
s := s2.WrapStream(stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since s is only used in the Handshake method, I would suggest nesting it there.

@@ -1080,6 +1084,25 @@ func (c *connectionNotifier) Connected(_ network.Network, _ network.Conn) {
c.metrics.HandledConnectionCount.Inc()
}

func newStreamMetricNotify(m metrics) *streamNotifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name it newStreamNotifier.

@notanatol notanatol merged commit 458a41a into master Nov 1, 2022
@notanatol notanatol deleted the feat-stream-metrics branch November 1, 2022 16:47
notanatol added a commit that referenced this pull request Nov 2, 2022
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase visibility into stream creation and closing
5 participants