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

Add a bit of logging #301

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Add a bit of logging #301

merged 2 commits into from
Dec 7, 2021

Conversation

hannahhoward
Copy link
Collaborator

Goals

These are somewhat analogous to tracing work but we already have comparable lines in the request manager and it feels reasonable to have this, since they've already proven useful there :)

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, although it's a bit more than logging! Is the ipr.state == graphsync.Running -> continue hypothetical and just a safety valve or are we seeing this in action? Is this related to the set of concerns you're wanting to solve with UUIDs?

@hannahhoward
Copy link
Collaborator Author

@rvagg this is totally a good point. The reality is I SUSPECT this may be happening in the real world --- I saw things in the Estuary logs. I think the first step is to remove the continue and just make it a log to answer if this is a real world problem. if it is, actually we may need to investigate root causes and resolve, rather than simply put the continue there. Probably having the continue is the right thing eventually, but it shouldn't be happening to begin with, and I'm not totally sure about the implications of that continue. So to be conservative, I'm gonna take it out and just make this informational.

@hannahhoward hannahhoward changed the base branch from feat/peer-stats to main December 7, 2021 05:38
@hannahhoward hannahhoward merged commit 6aa14b2 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/response-manager-logging 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.

2 participants