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

Peer Stats function #298

Merged
merged 3 commits into from
Dec 7, 2021
Merged

Peer Stats function #298

merged 3 commits into from
Dec 7, 2021

Conversation

hannahhoward
Copy link
Collaborator

Goals

fix #296

Implementation

This is just a WIP implementation. Have several things to decide:

  1. Interface types seem less than ideal. IN particular, worried about GC leaks as that's been a problem in the past. May rejigger.

  2. I think we may want to put this on impl/graphsync only for the time being?

Still messing around, obviously need to test.

@rvagg
Copy link
Member

rvagg commented Dec 3, 2021

Seems fine to me, although using the whole GraphSyncRequest and adding even more to it does feel heavier than maybe it needs to be. This probably depends largely on how we see it being consumed. The only mutable thing in here is the state, the other things are provided by the API consumer already (although I guess it's proxied through datatransfer so maybe gets lost?).

What would we be missing that we'd really need if we just made it PeerStates() and return a struct with two []RequestStates?

@hannahhoward
Copy link
Collaborator Author

upon further reflection I concur, though I think I'll make it map[requestID]state

add method that gets current request states by request ID for a given peer
@hannahhoward hannahhoward marked this pull request as ready for review December 4, 2021 02:34
@hannahhoward
Copy link
Collaborator Author

Re did this PR and added a bit of testing. I did move the method off the public interface for now. I know we're gonna rewrite this when we start doing big refactors so I'd prefer that no one get super attached :)

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

lgtm
But, why are the blocks in requestmanager/client.go and requestmanager/messages.go getting flagged by codecov for not being exercised by tests—shouldn't that impl/graphsync_test.go addition be covering these like it is for responsemanager?

Copy link
Collaborator Author

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

I think I spotted the source of your CodeCov issue though I agree it's weird the top level test doesn't catch that.

requestmanager/requestmanager_test.go Outdated Show resolved Hide resolved
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌

@hannahhoward hannahhoward merged commit b025c64 into main Dec 7, 2021
hannahhoward pushed a commit that referenced this pull request Dec 9, 2021
feat: add WorkerTaskQueue#WaitForNoActiveTasks() for tests (#284)

* feat: add WorkerTaskQueue#WaitForNoActiveTasks() for tests

* fixup! feat: add WorkerTaskQueue#WaitForNoActiveTasks() for tests

fix(responsemanager): fix flaky tests

fix(responsemanager): make fix more global

feat: add basic OT tracing for incoming requests

Closes: #271

docs(tests): document tracing test helper utilities

fix(test): increase 1s timeouts to 2s for slow CI (#289)

* fix(test): increase 1s timeouts to 2s for slow CI

* fixup! fix(test): increase 1s timeouts to 2s for slow CI

testutil/chaintypes: simplify maintenance of codegen (#294)

"go generate" now updates the generated code for us.

The separate directory for a main package was unnecessary;
a build-tag-ignored file is enough.

Using gofmt on the resulting source is now unnecessary too,
as upstream has been using go/format on its output for some time.

Finally, re-generate the output source code,
as the last time that was done we were on an older ipld-prime.

ipldutil: use chooser APIs from dagpb and basicnode (#292)

Saves us a bit of extra code, since they were added in summer.
Also avoid making defaultVisitor a variable,
which makes it clearer that it's never a nil func.

While here, replace node/basic with node/basicnode,
as the former has been deprecated in favor of the latter.

Co-authored-by: Hannah Howard <hannah@hannahhoward.net>

fix: use sync.Cond to handle no-task blocking wait (#299)

Ref: #284

Peer Stats function (#298)

* feat(graphsync): add impl method for peer stats

add method that gets current request states by request ID for a given peer

* fix(requestmanager): fix tested method

Add a bit of logging (#301)

* chore(responsemanager): add a bit of logging

* fix(responsemanager): remove code change

chore: short-circuit unnecessary message processing

Expose task queue diagnostics (#302)

* feat(impl): expose task queue diagnostics

* refactor(peerstate): put peerstate in its own module

* refactor(peerstate): make diagnostics return array
@mvdan mvdan deleted the feat/peer-stats branch December 15, 2021 14:17
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.

On demand peer based metrics
2 participants