Copilot commented on code in PR #212:
URL: https://github.com/apache/skywalking-rover/pull/212#discussion_r3526367026
##########
configs/rover_configs.yaml:
##########
@@ -161,6 +161,6 @@ access_log:
pprof:
# Is active the pprof
- active: ${ROVER_PPROF_ACTIVE:false}
+ active: ${ROVER_PPROF_ACTIVE:true}
# The bind port of the pprof HTTP server
Review Comment:
pprof is now enabled by default (`ROVER_PPROF_ACTIVE:true`). The pprof
module binds to all interfaces (Addr is `":<port>"` in pkg/pprof/module.go), so
this change can unintentionally expose `/debug/pprof/*` endpoints in
production. It also conflicts with the documentation default of `false`
(docs/en/setup/configuration/pprof.md).
##########
pkg/tools/process/process.go:
##########
@@ -61,26 +61,37 @@ func KernelFileProfilingStat() (*profiling.Info, error) {
return kernelFinder.Analyze(profiling.KernelProcSymbolFilePath)
}
-// ProfilingStat is validating the exe file could be profiling and get info
-func ProfilingStat(pid int32, exePath string) (*profiling.Info, error) {
+// SupportProfiling reports whether the executable can be profiled (not
excluded and has a usable
+// symbol table) WITHOUT retaining the parsed symbol/module data in memory.
Use this for the
+// discovery-time "support_ebpf_profiling" flag: the heavy symbol data is only
needed while an
+// actual profiling task runs and is built on demand by ProfilingStat.
+func SupportProfiling(exePath string) (bool, error) {
stat, err := os.Stat(exePath)
if err != nil {
- return nil, fmt.Errorf("check file error: %v", err)
+ return false, fmt.Errorf("check file error: %v", err)
}
for _, notSupport := range NotSupportProfilingExe {
if strings.HasPrefix(stat.Name(), notSupport) {
- return nil, fmt.Errorf("not support %s language
profiling", notSupport)
+ return false, fmt.Errorf("not support %s language
profiling", notSupport)
}
}
- context := newAnalyzeContext()
- // the executable file must have the symbols
- symbols, err := context.GetFinder(exePath).AnalyzeSymbols(exePath)
+ // the executable file must have the symbols; the parsed symbols are
discarded here so they
+ // are not retained in memory for every discovered process.
+ symbols, err :=
newAnalyzeContext().GetFinder(exePath).AnalyzeSymbols(exePath)
if err != nil || len(symbols) == 0 {
- return nil, fmt.Errorf("could not found any symbol in the
execute file: %s, error: %v", exePath, err)
+ return false, fmt.Errorf("could not found any symbol in the
execute file: %s, error: %v", exePath, err)
+ }
Review Comment:
SupportProfiling returns an error string that can include `error: <nil>`
when AnalyzeSymbols succeeds but returns zero symbols (because `err` is nil but
still formatted). Splitting the error cases also lets the message read more
clearly (and avoids the grammar issue "could not found").
##########
pkg/tools/process/process.go:
##########
@@ -61,26 +61,37 @@ func KernelFileProfilingStat() (*profiling.Info, error) {
return kernelFinder.Analyze(profiling.KernelProcSymbolFilePath)
}
-// ProfilingStat is validating the exe file could be profiling and get info
-func ProfilingStat(pid int32, exePath string) (*profiling.Info, error) {
+// SupportProfiling reports whether the executable can be profiled (not
excluded and has a usable
+// symbol table) WITHOUT retaining the parsed symbol/module data in memory.
Use this for the
+// discovery-time "support_ebpf_profiling" flag: the heavy symbol data is only
needed while an
+// actual profiling task runs and is built on demand by ProfilingStat.
+func SupportProfiling(exePath string) (bool, error) {
stat, err := os.Stat(exePath)
if err != nil {
- return nil, fmt.Errorf("check file error: %v", err)
+ return false, fmt.Errorf("check file error: %v", err)
}
for _, notSupport := range NotSupportProfilingExe {
if strings.HasPrefix(stat.Name(), notSupport) {
- return nil, fmt.Errorf("not support %s language
profiling", notSupport)
+ return false, fmt.Errorf("not support %s language
profiling", notSupport)
}
}
- context := newAnalyzeContext()
- // the executable file must have the symbols
- symbols, err := context.GetFinder(exePath).AnalyzeSymbols(exePath)
+ // the executable file must have the symbols; the parsed symbols are
discarded here so they
+ // are not retained in memory for every discovered process.
+ symbols, err :=
newAnalyzeContext().GetFinder(exePath).AnalyzeSymbols(exePath)
if err != nil || len(symbols) == 0 {
- return nil, fmt.Errorf("could not found any symbol in the
execute file: %s, error: %v", exePath, err)
+ return false, fmt.Errorf("could not found any symbol in the
execute file: %s, error: %v", exePath, err)
+ }
+ return true, nil
+}
+
+// ProfilingStat is validating the exe file could be profiling and get info
+func ProfilingStat(pid int32, exePath string) (*profiling.Info, error) {
+ if support, err := SupportProfiling(exePath); !support {
+ return nil, err
}
- return analyzeProfilingInfo(context, pid)
+ return analyzeProfilingInfo(newAnalyzeContext(), pid)
Review Comment:
ProfilingStat currently calls SupportProfiling(), which parses ELF symbols
via AnalyzeSymbols() using a new analyzeContext, and then ProfilingStat creates
another analyzeContext and calls analyzeProfilingInfo(). This can cause the
executable's symbols/finder to be parsed twice per profiling task, adding
avoidable CPU overhead.
--
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]
