cmd/top: First draft, have some issues #801
Conversation
cmd/incus/main.go
Outdated
| @@ -11,7 +11,7 @@ import ( | |||
|
|
|||
| "github.com/spf13/cobra" | |||
|
|
|||
| "github.com/lxc/incus/v6/client" | |||
| incus "github.com/lxc/incus/v6/client" | |||
There was a problem hiding this comment.
Tiny detail but this change should not be included, vscode tends to do that through running goimports. incus is already the package name for that URL so putting in directly as the import name is needless duplication.
cmd/incus/top.go
Outdated
| func (c *cmdTop) Run(cmd *cobra.Command, args []string) error { | ||
| conf := c.global.conf | ||
|
|
||
| remote, path, err := conf.ParseRemote("/1.0/metrics") |
There was a problem hiding this comment.
So I'm not too sure what's going on here.
conf.ParseRemote should be passed args[0] as is usual as that just points it to the correct server (e.g. incus top my-cluster:)
There was a problem hiding this comment.
The path part is actually irrelevant in this case, you only really need remote.
There was a problem hiding this comment.
If no argument is passed, should we assume that the remote is local then?
There was a problem hiding this comment.
Yeah, you can look at monitor.go for an example command with similar argument handling.
cmd/incus/top.go
Outdated
| // Validate path | ||
| if !strings.HasPrefix(path, "/") { | ||
| return fmt.Errorf(i18n.G("Query path must start with /")) | ||
| } | ||
|
|
There was a problem hiding this comment.
That's not relevant and can be dropped.
cmd/incus/top.go
Outdated
| return err | ||
| } | ||
|
|
||
| // Perform the query |
There was a problem hiding this comment.
So this part is odd as you're trying to directly hit /1.0/metrics to fetch the information when you could just run d.GetMetrics() to get the same data.
There was a problem hiding this comment.
Ah silly miss of mine- didn't realize d.GetMetrics existed. Will change this accordingly.
|
Left a bunch of comments in-line. |
|
Just modified it to address these issues. Also, can this PR be linked to #701? Left a few comments there about some assumptions I made. |
|
UI should update in realtime now like Small aside, I'm currently reading user shortcuts from stdin via bufio which requires shortcuts like 'd' (changing delay) to need an "enter" keypress to be registered. There are libraries like Also, I'm doing parsing for metrics into the MetricSet struct (locally defined) in this code - it could be useful to move this to an external file for future use involving metrics. |
Plain bufio is probably fine for now, keeping external dependencies down. |
|
Looks like |
cmd/incus/top.go
Outdated
| } | ||
|
|
||
| func (ms *MetricSet) getMetricValue(metricType metrics.MetricType, instanceName string) float64 { | ||
| value := 0.0 |
cmd/incus/top.go
Outdated
| global *cmdGlobal | ||
| } | ||
|
|
||
| type MetricSet struct { |
There was a problem hiding this comment.
Since it's used internally only it doesn't need to be exported, so should just be metricSet
cmd/incus/top.go
Outdated
|
|
||
| "github.com/spf13/cobra" | ||
|
|
||
| incus "github.com/lxc/incus/v6/client" |
There was a problem hiding this comment.
This should be import as just:
"github.com/lxc/incus/v6/client"
cmd/incus/top.go
Outdated
| incus "github.com/lxc/incus/v6/client" | ||
| cli "github.com/lxc/incus/v6/internal/cmd" | ||
| "github.com/lxc/incus/v6/internal/i18n" | ||
| "github.com/lxc/incus/v6/internal/server/metrics" |
There was a problem hiding this comment.
What are you getting from this one?
The client should normally never import anything from internal/server/XYZ
There was a problem hiding this comment.
This brings in the MetricType struct and MetricNames enum that I'm using to work with the logs. I can also copy the file over here if we want - let me know if this is preferred!
cmd/incus/top.go
Outdated
| cmd.Short = i18n.G("Manage command aliases") | ||
| cmd.Long = cli.FormatSection(i18n.G("Description"), i18n.G( | ||
| `Manage command aliases`)) |
There was a problem hiding this comment.
That description is incorrect :)
cmd/incus/top.go
Outdated
| return err | ||
| } | ||
|
|
||
| // // Start the ticker for periodic updates |
There was a problem hiding this comment.
Weird command identifier, probably just need one // :)
cmd/incus/top.go
Outdated
| kv := strings.Split(pair, "=") | ||
| if len(kv) != 2 { | ||
| continue | ||
| } |
cmd/incus/top.go
Outdated
| key := strings.TrimSpace(kv[0]) | ||
| value := strings.Trim(kv[1], "\"") | ||
| labels[key] = value | ||
| } |
cmd/incus/top.go
Outdated
| if typName == name { | ||
| return typ, true | ||
| } | ||
| } |
|
Accidentally closed this branch, replaced with #817 |
Working on adding support from
incus top, but having some issues:1.0/metricsusing the internal RawQuery mechanism, err is always not nil due to an invalid character being recognizedcmd/queryis structured, I try to fallback to raw text if RawQuery doesn't work. Is there a way to Unmarshal OpenMetrics structured strings into themetricsstruct?Would appreciate any guidance!