-
Notifications
You must be signed in to change notification settings - Fork 467
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 inspector command for system inspection #2663
Conversation
commands/diagnostic.go
Outdated
return nil | ||
} | ||
|
||
func filPath() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: This won't work if a user specified home directory with the --repodir
flag, you should parse that arg, import repo
and use repo.GetRepoDir(repoDirArgString)
function as in other commands. Also this way the env var stuff is taken care of for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved after blocking comment is addressed. The thing that worries me most about this command is the proliferation of abstractions on the CLI. It seems like the existence of this command, particularly the networking subcommand implies we should be rearranging existing commands underneath it like go-filecoin stats
, go-filecoin id
. In general this set of functionality helps more than it harms as most of the functionality introduced is needed and does not exist in another form on the CLI.
I do not support completing the rest of #2652 as it is written right now (config, network, filecoin) without rethinking existing commands or understanding how the rest of the proposed commands fit into the CLI as a whole. With this in mind I am imagining diag sys
becoming the top level command with disk, env, mem, net and runtime subcommands.
Are |
45ad752
to
bd9059e
Compare
2f6d197
to
705f567
Compare
@mishmosh I was originally borrowing from the naming convention used by go-ipfs, however I have altered this in attempt to be a bit more descriptive. The PR description now reflects this. @ZenGround0 I have re-written this inlight of your review to use plumbing, I think |
2dd3508
to
6e25c93
Compare
6e25c93
to
bc64a95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some linting comments as it's currently broken till #2650 lands
plumbing/inspector/service.go
Outdated
repo repo.Repo | ||
} | ||
|
||
// RuntimeInfor contains information about the golang runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeInfor
plumbing/inspector/service.go
Outdated
} | ||
|
||
// RuntimeInfor contains information about the environment filecoin is running in. | ||
type EnvironmentInfo struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnvironmentInfo contains
plumbing/inspector/service.go
Outdated
} | ||
} | ||
|
||
func (g *Service) Environment() *EnvironmentInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a doc comment
5f9fa35
to
2d9dc13
Compare
2d9dc13
to
fe9b7c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Please follow up with an update to the GitHub bug issue template so we can see this in action.
I think this is a nice clean example of something that does not need to be part of plumbing: it's not a core interface/component to be used by lots of higher level things. The alternative here is to put the inspector service on the command Env
directly (runAPIAndWait
in daemon.go) and bypass plumbing.
LGTM whether or not you do that here.
Do we want an |
This is a much better idea imo, I have modified it to do such.
Done: go-filecoin inspect all | jq
{
"Config": {
"api": {
"accessControlAllowCredentials": false,
"accessControlAllowMethods": [
"GET",
"POST",
"PUT"
],
"accessControlAllowOrigin": [
"http://localhost:8080",
"https://localhost:8080",
"http://127.0.0.1:8080",
"https://127.0.0.1:8080"
],
"address": "/ip4/127.0.0.1/tcp/3453"
},
"bootstrap": {
"addresses": [],
"minPeerThreshold": 0,
"period": "1m"
},
"datastore": {
"path": "badger",
"type": "badgerds"
},
"heartbeat": {
"beatPeriod": "3s",
"beatTarget": "",
"nickname": "",
"reconnectPeriod": "10s"
},
"metrics": {
"prometheusEnabled": false,
"prometheusEndpoint": "/ip4/0.0.0.0/tcp/9400",
"reportInterval": "5s"
},
"mining": {
"autoSealIntervalSeconds": 120,
"minerAddress": "empty",
"storagePrice": "0"
},
"mpool": {
"maxNonceGap": "100",
"maxPoolSize": 10000
},
"net": "",
"sectorbase": {
"rootdir": ""
},
"swarm": {
"address": "/ip4/0.0.0.0/tcp/6000"
},
"wallet": {
"defaultAddress": "t1ciamdwkjhbt7fjqm4chc3logy76cjbjey75qfia"
}
},
"Disk": {
"FSType": "61267",
"Free": 856211677184,
"Total": 805000208384
},
"Environment": {
"FIL_API": "",
"FIL_PATH": "",
"GOPATH": "/home/frrist"
},
"Memory": {
"Swap": 0,
"Virtual": 10331684000
},
"Runtime": {
"Arch": "amd64",
"Compiler": "gc",
"GoMaxProcs": 8,
"NumCGoCalls": 9,
"NumGoRoutines": 87,
"NumProc": 8,
"OS": "linux",
"Version": "go1.12.1"
}
} |
Sounds good, will do this once it merges |
* add inspector command and tests
Description
This PR makes progress towards completing #2652 by adding the following commands:
Disk: Prints out information about the filesystem
Memory: Prints out information about system memory.
Runtime: Prints out information about your golang runtime.
Environment: Prints out information about your filecoin nodes environment.
Config: Prints out information about your filecoin nodes in memory config.