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

nodes status should include connection and graph stats #52

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

addievo
Copy link
Contributor

@addievo addievo commented Nov 5, 2023

Description

Needs a rebase after merge of #45

Currently, nodes status lacks a lot of vital information, this PR aims to add some extra information to nodes status to make it more informational.

Currently, we have added

  • Number of active connections.
  • Total nodes created.

Issues Fixed

Tasks

  • 1. Implement active connections.
  • 2. Implement total nodes.
  • 3. Implement node graph information other than total nodes.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@ghost
Copy link

ghost commented Nov 5, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@addievo addievo assigned addievo and amydevs and unassigned addievo Nov 5, 2023
@addievo addievo mentioned this pull request Nov 5, 2023
@CMCDragonkai CMCDragonkai assigned tegefaulkes and unassigned amydevs Nov 14, 2023
@CMCDragonkai
Copy link
Member

If this only requires minor changes, then we should just get this in @tegefaulkes

@tegefaulkes
Copy link
Contributor

I've rebased.

Looking over this, I think that it needs a bit more work. The agent status handler needs to be updated to provide the stats. This needs to be implemented in Polykey. Right now the command does separate RPC calls to collect the stats.

@CMCDragonkai
Copy link
Member

You can go ahead and work on this if you can merge in today. We got other stuff to do too.

@tegefaulkes
Copy link
Contributor

I'm adding some useful stats to the NodeConnectionManager.

connectionsActive
connectionsMadeForward
connectionsMadeReverse
connectionsUsage
connectionsAcquired
streamsMadeForward
streamsMadeReverse
streamsActive

@CMCDragonkai
Copy link
Member

Are you going to store these stats somewhere? Or are they just counts of whatever state we have? Cause I'd want to avoid storing any state, where it would be more appropriate for the audit domain in MatrixAI/Polykey#628

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Nov 15, 2023

I'm just adding nodes stats for now. We can expand on this later with other stats if anything else seems useful while testing.

They're just in-memory counts for now. anything persistent would be for the audit domain.

@tegefaulkes
Copy link
Contributor

I'm considering this mostly done now.

When I did the rebase I cut out a bunch of changes that didn't make sense given the scope of this PR. I need to go over the old code which I saved in feature-cli-polish-2-BACKUP branch and see what's actually needed. I need some information on what 8b1191c was trying to do. It conflicted with a lot of changes in staging and there is no mention of this in the PR or issue.

@CMCDragonkai
Copy link
Member

@addievo can you provide commentary about 8b1191c and @tegefaulkes make a judgement call on what's needed.

@CMCDragonkai
Copy link
Member

And if so, merge and go ahead to the nodes hole punching refactoring!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

nodes status command should include connection and graph stats
4 participants