Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support Async Profiler Feature #203
Support Async Profiler Feature #203
Changes from 3 commits
d2355a0
37550c6
8e10fe0
50b7ba4
5694b3a
b8af7e4
ecd88bd
54d3486
071d2f1
ddb13fa
693a787
d4222fb
5a38d58
4e144c7
ac0314b
d9e2e71
8395933
63167d9
3eedb05
8a00dee
0f6665c
5f9bdc0
ba0d366
c1b9519
a20535e
b4c5cf7
9593efc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If this command is under the
profiling
command, should we remove theprofiler
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?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.
Please update the usage text.
Check failure on line 3 in internal/commands/profiling/asyncprofiler/create.go
GitHub Actions / Golang Lint
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.
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.
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.
Should we make the type of
execArgs
as*string
, and assign the value when judgment thectx.String
is not an empty string? You will get an empty string even if theexec-args
is not declared.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.
We usually not using the
StringSlice
to get slice value, I think we should just change back to usingString
and split the value by self(,
)?Check failure on line 3 in internal/commands/profiling/asyncprofiler/getAnalyze.go
GitHub Actions / Golang Lint
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.
The enum should be a new model. Instant of a string value without any type check.
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.
Please use
String
and split by self.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.
Let us declare the
startTime
,endTime
, andlimit
as*int64
or*int
first, otherwise the value will be0
.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.
Can it be used as
swctl profiling asyncprofiler logs
? I think it's a difference in the meaning of progress.