Support Async Profiler Feature by zhengziyi0117 · Pull Request #203 · apache/skywalking-cli

zhengziyi0117 · 2024-10-24T13:33:51Z

Add the sub-command profiling async for async-profiler query API

Here are some examples

# Create a async-profiler task
swctl profiling async create --service-name=service-name --duration=60 --events=cpu,alloc \ 
	--instance-name-list=instance-name1,instance-name2 --exec-args=interval=50ms
# Query all async-profiler tasks
swctl profiling async list --service-name=service-name
# Query task progress, including task logs and successInstances and errorInstances
swctl profiling async progress --task-id=task-id
# Query the flame graph produced by async-profiler
swctl profiling async analysis --service-name=service-name --task-id=task-id \
	--instance-name-list=instance-name1,instance-name2 --event=execution_sample

wu-sheng · 2024-10-24T13:38:17Z

@mrproliu We don't have backend ready, as we need this to verify the backend. Please review from codes as much as possible.

wu-sheng · 2024-10-24T13:38:37Z

@zhengziyi0117 Please fix CI.

mrproliu · 2024-10-25T00:28:54Z

+var Command = &cli.Command{
+	Name:      "asyncprofiler",
+	Usage:     "async profiler related sub-command",
+	UsageText: `todo`,


Please update the usage text.

mrproliu · 2024-10-25T00:31:45Z

+)
+
+var Command = &cli.Command{
+	Name:      "asyncprofiler",


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?

mrproliu · 2024-10-25T00:36:25Z

+			eventTypes = append(eventTypes, query.AsyncProfilerEventType(upperCaseEvent))
+		}
+
+		execArgs := ctx.String("exec-args")


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.

mrproliu · 2024-10-25T00:44:10Z

+		eventTypes := make([]query.AsyncProfilerEventType, 0)
+		for _, event := range events {
+			upperCaseEvent := strings.ToUpper(event)
+			eventTypes = append(eventTypes, query.AsyncProfilerEventType(upperCaseEvent))
+		}


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.

mrproliu · 2024-10-25T00:50:44Z

+			&cli.StringFlag{
+				Name:     "event",
+				Usage:    "which event types this task needs to collect.",
+				Required: true,
+			},


The enum should be a new model. Instant of a string value without any type check.

mrproliu · 2024-10-25T00:51:13Z

+
+		request := &query.AsyncProfilerAnalyzationRequest{
+			TaskID:      ctx.String("task-id"),
+			InstanceIds: ctx.StringSlice("service-instance-ids"),


Please use String and split by self.

mrproliu · 2024-10-25T00:55:17Z

+		startTime := ctx.Int64("start-time")
+		endTime := ctx.Int64("end-time")
+		limit := ctx.Int("limit")


Let us declare the startTime, endTime, and limit as *int64 or *int first, otherwise the value will be 0.

mrproliu · 2024-10-25T00:57:13Z

+
+var getTaskProgressCommand = &cli.Command{
+	Name:    "progress",
+	Aliases: []string{"logs", "p"},


Can it be used as swctl profiling asyncprofiler logs? I think it's a difference in the meaning of progress.

mrproliu · 2024-10-25T01:20:05Z

@zhengziyi0117 Also, please update the CHANGES.md

Co-authored-by: mrproliu <741550557@qq.com>

lujiajing1126 · 2024-10-26T07:40:59Z

Pls run golangci-lint locally to ensure it can pass CI

mrproliu · 2024-10-27T14:15:13Z

+	instanceNameFlagName      = "instance-name"
+	destInstanceIDFlagName    = "dest-instance-id"
+	destInstanceNameFlagName  = "dest-instance-name"
+	InstanceIDSliceFlagName   = "instance-id-slice"


We usually use "list" instant of "slice"

mrproliu · 2024-10-27T14:20:33Z

+Examples:
+1. Query the flame graph produced by async-profiler
+$ swctl profiling async analysis --service-name=service-name --task-id=task-id \
+	--service-instance-ids=instance-name1,instance-name2 --event=execution_sample`,


Please update all incorrect name of the parameter.

wu-sheng · 2024-10-28T02:08:44Z

Could you update the PR description to add which commands are you adding, and put some examples(input and output) in it?

zhengziyi added 3 commits October 23, 2024 19:57

feat: add asyncprofiler support

d2355a0

fix: alias

37550c6

feat: support asyncprofiler

8e10fe0

wu-sheng requested a review from mrproliu October 24, 2024 13:37

wu-sheng added this to the 0.15.0 milestone Oct 24, 2024

wu-sheng added the feature this pull request provides new feature label Oct 24, 2024

mrproliu reviewed Oct 25, 2024

View reviewed changes

mrproliu and others added 16 commits October 25, 2024 09:20

Merge branch 'master' into master

50b7ba4

Update assets/graphqls/profiling/asyncprofiler/GetTaskList.graphql

5694b3a

Co-authored-by: mrproliu <741550557@qq.com>

Update internal/commands/profiling/asyncprofiler/getAnalyze.go

b8af7e4

Co-authored-by: mrproliu <741550557@qq.com>

fix: license header

ecd88bd

Merge remote-tracking branch 'origin/master'

54d3486

Update internal/commands/profiling/asyncprofiler/getTaskProgress.go

071d2f1

Co-authored-by: mrproliu <741550557@qq.com>

Merge remote-tracking branch 'origin/master'

ddb13fa

fix: pr comment

693a787

fix: license header

d4222fb

fix: license header

5a38d58

fix: license header

4e144c7

test: golint version

ac0314b

test: golint version

d9e2e71

fix: go ci lint

8395933

feat: add instanceSlice interceptor

63167d9

feat: update CHANGES.md

3eedb05

lujiajing1126 and others added 3 commits October 26, 2024 16:08

fix style and lint

8a00dee

fix: golint

0f6665c

fix: golint

5f9bdc0

fix: command test ci

ba0d366

mrproliu reviewed Oct 27, 2024

View reviewed changes

zhengziyi and others added 4 commits October 28, 2024 00:04

fix: arguments name

c1b9519

fix: arguments name

a20535e

fix dependency test cases

b4c5cf7

fix trace cases

9593efc

lujiajing1126 requested a review from mrproliu October 28, 2024 02:08

mrproliu approved these changes Oct 28, 2024

View reviewed changes

mrproliu merged commit bce7faa into apache:master Oct 28, 2024

wu-sheng mentioned this pull request Nov 30, 2024

Update CHANGES #206

Merged

Conversation

zhengziyi0117 commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented Oct 24, 2024

Uh oh!

wu-sheng commented Oct 24, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrproliu commented Oct 25, 2024

Uh oh!

lujiajing1126 commented Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Oct 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhengziyi0117 commented Oct 24, 2024 •

edited

Loading

lujiajing1126 commented Oct 26, 2024 •

edited

Loading