feat(add trace profile for go agent) #229
Conversation
go.mod
Outdated
| module github.com/apache/skywalking-go | ||
|
|
||
| go 1.19 | ||
| go 1.24 |
There was a problem hiding this comment.
Why does the agent need to update to 1.24? The user may use the older go version and it will break the user dependency.
go.mod
Outdated
| toolchain go1.24.4 | ||
|
|
||
| replace ( | ||
| skywalking.apache.org/repo/goapi => ../skywalking-goapi |
There was a problem hiding this comment.
Please update the goapi back to the original version.
| skywalking.apache.org/repo/goapi => ../skywalking-goapi | ||
| ) | ||
|
|
||
| require ( |
There was a problem hiding this comment.
This feature no needs to update the dependency, please roll back. Only goapi should be updated.
plugins/core/tracing/api.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| //id := span.(AdaptSpan).GetTraceID() |
There was a problem hiding this comment.
Please remove the comment line.
plugins/echov4/intercepter.go
Outdated
| } | ||
|
|
||
| id := span.TraceID() | ||
| fmt.Println("trace_id:", id) |
There was a problem hiding this comment.
The fmt.Println should be remove.
| const grpcReporterInitFunc = ` | ||
| func initGRPCReporter(logger operator.LogOperator, | ||
| checkInterval time.Duration, |
There was a problem hiding this comment.
Please keep the original format.
…om:cqhasy/skywalking-go into main
… the profile task checking order)
.github/workflows/plugin-tests.yaml
Outdated
|
|
||
| on: | ||
| pull_request: | ||
| push: |
There was a problem hiding this comment.
Please keep this only trigger on the pull request to reduce the CI resource.
|
|
||
| on: | ||
| push: | ||
| pull_request: |
There was a problem hiding this comment.
Please rollback this change.
plugins/core/prof_labels.go
Outdated
| p := runtimeGetProfLabel() | ||
| if p != nil { | ||
| version := runtime.Version() | ||
| if version < "go1.20" { |
There was a problem hiding this comment.
Why do you compare the Go version as a string? It should be an int type?
plugins/core/operator/profiler.go
Outdated
|
|
||
| type ProfileOperator interface { | ||
| GetPprofLabelSet(traceID string, segmentID string, spanID int32) interface{} | ||
| TurnToPprofLabel(l interface{}) interface{} |
There was a problem hiding this comment.
Can we simplify the profile operator? We should only set and get the trace info from the goroutine is enough.
There was a problem hiding this comment.
Seems like these two methods are only invoked by one place. Can we combine them to one method? And could you explain what these are to the methods work for?
|
|
||
| func (i *Instrument) FilterAndEdit(path string, curFile *dst.File, cursor *dstutil.Cursor, allFiles []*dst.File) bool { | ||
| // skip stdlib/module cache | ||
| if i.opts != nil { |
There was a problem hiding this comment.
Looks like you want to enhance a lot of packages? why you needs to do this?
plugins/core/profile.go
Outdated
| if _, exists := m.TraceProfileTasks[task.TaskID]; exists { | ||
| return | ||
| } | ||
| endTime := task.StartTime + int64(task.Duration)*60*1000 |
There was a problem hiding this comment.
Please using time.Time#Add to add duration.
plugins/core/profile.go
Outdated
| m.Log.Errorf("profile event error:%v", err) | ||
| return | ||
| } | ||
| t := m.TraceProfileTasks[m.currentTask.taskID] |
There was a problem hiding this comment.
Actually, this map will only have one data, so there's no need to use the map, right?
plugins/core/reporter/api.go
Outdated
| type TaskStatus int | ||
|
|
||
| const ( | ||
| Pending TaskStatus = iota |
There was a problem hiding this comment.
Does this status only work for the trace profiling task? Please add the prefix of the const. In case it conflicts with other concepts.
plugins/core/profile.go
Outdated
| defer ticker.Stop() | ||
|
|
||
| go func() { | ||
| for range ticker.C { |
There was a problem hiding this comment.
If the task has started, and after one second, the endpoint does not trigger any request. The agent will stop the profiling without reaching the end time? This logic may not be correct.
Also, if the end time is not reached, and you have stopped the profiling, at this time, the agent receives the request that matches the endpoint. Will it restart the profiling?
So I think for the simplify, you could just stop the profiling when the end time reached.
… pprof in internal)
.github/workflows/plugin-tests.yaml
Outdated
|
|
||
| on: | ||
| pull_request: | ||
| puLL_request: |
There was a problem hiding this comment.
Please rollback to the original one.
go.work
Outdated
| ./test/plugins/scenarios/logrus | ||
| ./test/plugins/scenarios/zap | ||
| ./test/plugins/scenarios/mux | ||
| ./plugins/pprof |
There was a problem hiding this comment.
Please update the locate same with other plugins.
plugins/core/operator/profiler.go
Outdated
|
|
||
| type ProfileOperator interface { | ||
| GetPprofLabelSet(traceID string, segmentID string, spanID int32) interface{} | ||
| TurnToPprofLabel(l interface{}) interface{} |
There was a problem hiding this comment.
Seems like these two methods are only invoked by one place. Can we combine them to one method? And could you explain what these are to the methods work for?
2 participants
No description provided.