mrproliu commented on code in PR #203:
URL: https://github.com/apache/skywalking-cli/pull/203#discussion_r1815840724
##########
internal/commands/profiling/asyncprofiler/asyncprofiler.go:
##########
@@ -0,0 +1,17 @@
+package asyncprofiler
+
+import (
+ "github.com/urfave/cli/v2"
+)
+
+var Command = &cli.Command{
+ Name: "asyncprofiler",
Review Comment:
If this command is under the `profiling` command, should we remove the
`profiler` suffix?
For now, the command should be: `swctl profiling asyncprofiler`, I think it
could be: `swctl profiling async` is much clear, what do you think?
##########
internal/commands/profiling/asyncprofiler/create.go:
##########
@@ -0,0 +1,76 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/commands/interceptor"
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var createCommand = &cli.Command{
+ Name: "create",
+ Aliases: []string{"c"},
+ Usage: "Create a new async profiler task",
+ UsageText: `Create a new async profiler task
+
+Examples:
+1. Create async-profiler task
+$ swctl profiling asyncprofiler create --service-name=someservicename
--duration=60 --events=cpu --service-instance-ids=someinstance`,
+ Flags: flags.Flags(
+ flags.ServiceFlags,
+ []cli.Flag{
+ &cli.StringSliceFlag{
+ Name: "service-instance-ids",
+ Usage: "which instances to execute task.",
+ Required: true,
+ },
+ &cli.IntFlag{
+ Name: "duration",
+ Usage: "task continuous time(second).",
+ Required: true,
+ },
+ &cli.StringSliceFlag{
+ Name: "events",
+ Usage: "which event types this task needs to
collect.",
+ Required: true,
+ },
+ &cli.StringFlag{
+ Name: "exec-args",
+ Usage: "other async-profiler execution options,
e.g. alloc=2k,lock=2s.",
+ },
+ },
+ ),
+ Before: interceptor.BeforeChain(
+ interceptor.ParseService(false),
+ ),
+ Action: func(ctx *cli.Context) error {
+ serviceID := ctx.String("service-id")
+
+ events := ctx.StringSlice("events")
+ eventTypes := make([]query.AsyncProfilerEventType, 0)
+ for _, event := range events {
+ upperCaseEvent := strings.ToUpper(event)
+ eventTypes = append(eventTypes,
query.AsyncProfilerEventType(upperCaseEvent))
+ }
+
+ execArgs := ctx.String("exec-args")
Review Comment:
Should we make the type of `execArgs` as `*string`, and assign the value
when judgment the `ctx.String` is not an empty string? You will get an empty
string even if the `exec-args` is not declared.
##########
internal/commands/profiling/asyncprofiler/getAnalyze.go:
##########
@@ -0,0 +1,59 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ query "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var analysisCommand = &cli.Command{
+ Name: "analysis",
+ Aliases: []string{"a"},
+ Usage: "Query async-profiler analysis",
+ UsageText: `Query async-profiler analysis
+
+Examples:
+1. Query the flame graph produced by async-profiler
+$ /swctl-dev-37550c6-darwin-arm64 profiling asyncprofiler analysis
--task-id=task-id --service-instance-ids=instanceIds --event=execution_sample`,
Review Comment:
```suggestion
$ swctl profiling asyncprofiler analysis --task-id=task-id
--service-instance-ids=instanceIds --event=execution_sample`,
```
##########
internal/commands/profiling/asyncprofiler/create.go:
##########
@@ -0,0 +1,76 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/commands/interceptor"
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var createCommand = &cli.Command{
+ Name: "create",
+ Aliases: []string{"c"},
+ Usage: "Create a new async profiler task",
+ UsageText: `Create a new async profiler task
+
+Examples:
+1. Create async-profiler task
+$ swctl profiling asyncprofiler create --service-name=someservicename
--duration=60 --events=cpu --service-instance-ids=someinstance`,
+ Flags: flags.Flags(
+ flags.ServiceFlags,
+ []cli.Flag{
+ &cli.StringSliceFlag{
+ Name: "service-instance-ids",
+ Usage: "which instances to execute task.",
+ Required: true,
+ },
+ &cli.IntFlag{
+ Name: "duration",
+ Usage: "task continuous time(second).",
+ Required: true,
+ },
+ &cli.StringSliceFlag{
+ Name: "events",
+ Usage: "which event types this task needs to
collect.",
+ Required: true,
+ },
+ &cli.StringFlag{
+ Name: "exec-args",
+ Usage: "other async-profiler execution options,
e.g. alloc=2k,lock=2s.",
+ },
+ },
+ ),
+ Before: interceptor.BeforeChain(
+ interceptor.ParseService(false),
+ ),
+ Action: func(ctx *cli.Context) error {
+ serviceID := ctx.String("service-id")
+
+ events := ctx.StringSlice("events")
+ eventTypes := make([]query.AsyncProfilerEventType, 0)
+ for _, event := range events {
+ upperCaseEvent := strings.ToUpper(event)
+ eventTypes = append(eventTypes,
query.AsyncProfilerEventType(upperCaseEvent))
+ }
Review Comment:
It is best to create an enum model to obtain the event types, and the CLI
should verify whether we have an enum value. You can refer to
https://github.com/apache/skywalking-cli/blob/master/internal/model/type.go.
However, the type you choose should be a slice, which is slightly different
from the existing reference.
##########
internal/commands/profiling/asyncprofiler/asyncprofiler.go:
##########
@@ -0,0 +1,17 @@
+package asyncprofiler
+
+import (
+ "github.com/urfave/cli/v2"
+)
+
+var Command = &cli.Command{
+ Name: "asyncprofiler",
+ Usage: "async profiler related sub-command",
+ UsageText: `todo`,
Review Comment:
Please update the usage text.
##########
assets/graphqls/profiling/asyncprofiler/GetTaskList.graphql:
##########
@@ -0,0 +1,31 @@
+# Licensed to Apache Software Foundation (ASF) under one or more contributor
+# license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright
+# ownership. Apache Software Foundation (ASF) licenses this file to you under
+# the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+query ($condition: AsyncProfilerTaskListRequest) {
Review Comment:
```suggestion
query ($condition: AsyncProfilerTaskListRequest!) {
```
Following the [query
protocol](https://github.com/apache/skywalking-query-protocol/blob/master/async-profiler.graphqls#L181),
I think the condition argument is required?
##########
internal/commands/profiling/asyncprofiler/getTaskProgress.go:
##########
@@ -0,0 +1,36 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+)
+
+var getTaskProgressCommand = &cli.Command{
+ Name: "progress",
+ Aliases: []string{"logs", "p"},
Review Comment:
Can it be used as `swctl profiling asyncprofiler logs`? I think it's a
difference in the meaning of progress.
##########
internal/commands/profiling/asyncprofiler/create.go:
##########
@@ -0,0 +1,76 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/commands/interceptor"
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var createCommand = &cli.Command{
+ Name: "create",
+ Aliases: []string{"c"},
+ Usage: "Create a new async profiler task",
+ UsageText: `Create a new async profiler task
+
+Examples:
+1. Create async-profiler task
+$ swctl profiling asyncprofiler create --service-name=someservicename
--duration=60 --events=cpu --service-instance-ids=someinstance`,
+ Flags: flags.Flags(
+ flags.ServiceFlags,
+ []cli.Flag{
+ &cli.StringSliceFlag{
+ Name: "service-instance-ids",
+ Usage: "which instances to execute task.",
+ Required: true,
+ },
+ &cli.IntFlag{
+ Name: "duration",
+ Usage: "task continuous time(second).",
+ Required: true,
+ },
+ &cli.StringSliceFlag{
+ Name: "events",
+ Usage: "which event types this task needs to
collect.",
+ Required: true,
+ },
+ &cli.StringFlag{
+ Name: "exec-args",
+ Usage: "other async-profiler execution options,
e.g. alloc=2k,lock=2s.",
+ },
+ },
+ ),
+ Before: interceptor.BeforeChain(
+ interceptor.ParseService(false),
+ ),
+ Action: func(ctx *cli.Context) error {
+ serviceID := ctx.String("service-id")
+
+ events := ctx.StringSlice("events")
+ eventTypes := make([]query.AsyncProfilerEventType, 0)
+ for _, event := range events {
+ upperCaseEvent := strings.ToUpper(event)
+ eventTypes = append(eventTypes,
query.AsyncProfilerEventType(upperCaseEvent))
+ }
+
+ execArgs := ctx.String("exec-args")
+
+ request := &query.AsyncProfilerTaskCreationRequest{
+ ServiceID: serviceID,
+ ServiceInstanceIds:
ctx.StringSlice("service-instance-ids"),
Review Comment:
We usually not using the `StringSlice` to get slice value, I think we should
just change back to using `String` and split the value by self(`,`)?
##########
internal/commands/profiling/asyncprofiler/getAnalyze.go:
##########
@@ -0,0 +1,59 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ query "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var analysisCommand = &cli.Command{
+ Name: "analysis",
+ Aliases: []string{"a"},
+ Usage: "Query async-profiler analysis",
+ UsageText: `Query async-profiler analysis
+
+Examples:
+1. Query the flame graph produced by async-profiler
+$ /swctl-dev-37550c6-darwin-arm64 profiling asyncprofiler analysis
--task-id=task-id --service-instance-ids=instanceIds --event=execution_sample`,
+ Flags: flags.Flags(
+ []cli.Flag{
+ &cli.StringFlag{
+ Name: "task-id",
+ Usage: "async-profiler task id",
+ Required: true,
+ },
+ &cli.StringSliceFlag{
+ Name: "service-instance-ids",
+ Usage: "select which service instances' jfr
files to analyze",
+ Required: true,
+ },
+ &cli.StringFlag{
+ Name: "event",
+ Usage: "which event types this task needs to
collect.",
+ Required: true,
+ },
Review Comment:
The enum should be a new model. Instant of a string value without any type
check.
##########
internal/commands/profiling/asyncprofiler/getAnalyze.go:
##########
@@ -0,0 +1,59 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ query "skywalking.apache.org/repo/goapi/query"
+ "strings"
+)
+
+var analysisCommand = &cli.Command{
+ Name: "analysis",
+ Aliases: []string{"a"},
+ Usage: "Query async-profiler analysis",
+ UsageText: `Query async-profiler analysis
+
+Examples:
+1. Query the flame graph produced by async-profiler
+$ /swctl-dev-37550c6-darwin-arm64 profiling asyncprofiler analysis
--task-id=task-id --service-instance-ids=instanceIds --event=execution_sample`,
+ Flags: flags.Flags(
+ []cli.Flag{
+ &cli.StringFlag{
+ Name: "task-id",
+ Usage: "async-profiler task id",
+ Required: true,
+ },
+ &cli.StringSliceFlag{
+ Name: "service-instance-ids",
+ Usage: "select which service instances' jfr
files to analyze",
+ Required: true,
+ },
+ &cli.StringFlag{
+ Name: "event",
+ Usage: "which event types this task needs to
collect.",
+ Required: true,
+ },
+ },
+ ),
+ Action: func(ctx *cli.Context) error {
+ event := ctx.String("event")
+ eventType := query.JFREventType(strings.ToUpper(event))
+
+ request := &query.AsyncProfilerAnalyzationRequest{
+ TaskID: ctx.String("task-id"),
+ InstanceIds: ctx.StringSlice("service-instance-ids"),
Review Comment:
Please use `String` and split by self.
##########
internal/commands/profiling/asyncprofiler/getTaskProgress.go:
##########
@@ -0,0 +1,36 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+)
+
+var getTaskProgressCommand = &cli.Command{
+ Name: "progress",
+ Aliases: []string{"logs", "p"},
+ Flags: []cli.Flag{
+ &cli.StringFlag{
+ Name: "task-id",
+ Usage: "async profiler task id.",
+ },
+ },
+ Usage: "Query async-profiler task progress",
+ UsageText: `Query async-profiler task progress
+
+Examples:
+1. Query task progress, including task logs and successInstances and
errorInstances
+$ swctl-dev-37550c6-darwin-arm64 profiling asyncprofiler progress
--task-id=task-id`,
Review Comment:
```suggestion
$ swctl profiling asyncprofiler progress --task-id=task-id`,
```
##########
internal/commands/profiling/asyncprofiler/getTaskList.go:
##########
@@ -0,0 +1,62 @@
+package asyncprofiler
+
+import (
+ "github.com/apache/skywalking-cli/internal/commands/interceptor"
+ "github.com/apache/skywalking-cli/internal/flags"
+ "github.com/apache/skywalking-cli/pkg/display"
+ "github.com/apache/skywalking-cli/pkg/display/displayable"
+ "github.com/apache/skywalking-cli/pkg/graphql/profiling"
+ "github.com/urfave/cli/v2"
+ "skywalking.apache.org/repo/goapi/query"
+)
+
+var getTaskListCommand = &cli.Command{
+ Name: "list",
+ Aliases: []string{"l"},
+ Usage: "Query async-profiler task list",
+ UsageText: `Query async-profiler task list
+
+Examples:
+1. Query all async-profiler tasks
+$ swctl profiling asyncprofiler list --service-name=TEST_AGENT`,
+ Flags: flags.Flags(
+ flags.ServiceFlags,
+ []cli.Flag{
+ &cli.Int64Flag{
+ Name: "start-time",
+ Usage: "The start time (in milliseconds) of the
event, measured between the current time and midnight, January 1, 1970 UTC.",
+ },
+ &cli.Int64Flag{
+ Name: "end-time",
+ Usage: "The end time (in milliseconds) of the
event, measured between the current time and midnight, January 1, 1970 UTC.",
+ },
+ &cli.IntFlag{
+ Name: "limit",
+ Usage: "Limit defines the number of the tasks
to be returned.",
+ },
+ },
+ ),
+ Before: interceptor.BeforeChain(
+ interceptor.ParseService(true),
+ ),
+ Action: func(ctx *cli.Context) error {
+ serviceID := ctx.String("service-id")
+ startTime := ctx.Int64("start-time")
+ endTime := ctx.Int64("end-time")
+ limit := ctx.Int("limit")
Review Comment:
Let us declare the `startTime`, `endTime`, and `limit` as `*int64` or `*int`
first, otherwise the value will be `0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
