PMM-5492 Add Pprof data to logs.zip #1117
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## main #1117 +/- ##
==========================================
- Coverage 49.29% 48.99% -0.31%
==========================================
Files 180 184 +4
Lines 20826 21519 +693
==========================================
+ Hits 10267 10543 +276
- Misses 9426 9794 +368
- Partials 1133 1182 +49
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
main.go
Outdated
| mux.HandleFunc("/logs.zip", func(rw http.ResponseWriter, req *http.Request) { | ||
| contextTimeout := 10 * time.Second |
There was a problem hiding this comment.
I propose to use just a constant for that.
There was a problem hiding this comment.
Agree with @artemgavrilov, no reason to artificially increase the number of items in configs.
There was a problem hiding this comment.
Moved to const defaultContextTimeout.
main.go
Outdated
| mux.HandleFunc("/logs.zip", func(rw http.ResponseWriter, req *http.Request) { | ||
| contextTimeout := 10 * time.Second |
There was a problem hiding this comment.
I propose to use just a constant for that.
main.go
Outdated
| mux.HandleFunc("/logs.zip", func(rw http.ResponseWriter, req *http.Request) { | ||
| contextTimeout := 10 * time.Second |
There was a problem hiding this comment.
Agree with @artemgavrilov, no reason to artificially increase the number of items in configs.
main.go
Outdated
| mux.HandleFunc("/logs.zip", func(rw http.ResponseWriter, req *http.Request) { | ||
| contextTimeout := 10 * time.Second | ||
| // increase context timeout if pprof query parameter exist in request | ||
| pprofQueryParameter, _ := strconv.Atoi(req.FormValue("pprof")) |
There was a problem hiding this comment.
I suggest using strconv.ParseBool instead of strconv.Atoi.
Do not ignore err, let's at least log the message.
services/config/pmm-managed.yaml
Outdated
| pprof: | ||
| profile_duration: 30s | ||
| trace_duration: 10s |
There was a problem hiding this comment.
making timeouts configurable doesn't bring any value to customers, it's so rare case when we might need to update these values in the future.
services/config/config.go
Outdated
| @@ -49,6 +50,7 @@ type Config struct { | |||
| Services struct { | |||
| Platform platform.Config `yaml:"platform"` | |||
| Telemetry telemetry.ServiceConfig `yaml:"telemetry"` | |||
| Pprof pprof.Config `yaml:"pprof"` | |||
There was a problem hiding this comment.
please remove it from the config, it will simplify the code a lot.
services/supervisord/logs.go
Outdated
| traceBytes, err := pprofUtils.Trace(pprofConfig.TraceDuration) | ||
| files = append(files, fileContent{ | ||
| Name: "pprof/trace.out", | ||
| Data: traceBytes, | ||
| Err: err, | ||
| }) | ||
|
|
||
| profileBytes, err := pprofUtils.Profile(pprofConfig.ProfileDuration) | ||
| files = append(files, fileContent{ | ||
| Name: "pprof/profile.pb.gz", | ||
| Data: profileBytes, | ||
| Err: err, | ||
| }) |
There was a problem hiding this comment.
can we run them in parallel to speed up?
| } | ||
| var pprofConfig *pprof.Config | ||
| if pprofQueryParameter { | ||
| contextTimeout += pProfProfileDuration + pProfTraceDuration |
There was a problem hiding this comment.
I guess nginx will terminate connections longer than 10 minutes. Can you please check it? If so we should document this limitation or change nginx configuration.
PMM-5492 Add Pprof data to logs.zip
Build: SUBMODULES-2574