Skip to content
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

Implement pprof module extension #3773

Merged
merged 7 commits into from
Jul 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 54 additions & 33 deletions api/service/pprof/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type Service struct {
var (
initOnce sync.Once
svc = &Service{}
cpuFile *os.File
)

// NewService creates the new pprof service
Expand All @@ -61,21 +62,21 @@ func NewService(cfg Config) *Service {

func newService(cfg Config) *Service {
if !cfg.Enabled {
utils.Logger().Info().Msg("Pprof service disabled...")
utils.Logger().Info().Msg("pprof service disabled...")
return nil
}

utils.Logger().Debug().Str("cfg", cfg.String()).Msg("Pprof")
utils.Logger().Debug().Str("cfg", cfg.String()).Msg("pprof")
svc.config = cfg

if profiles, err := cfg.unpackProfilesIntoMap(); err != nil {
log.Fatal("Could not unpack pprof profiles into map")
} else {
svc.profiles = profiles
profiles, err := cfg.unpackProfilesIntoMap()
if err != nil {
log.Fatal("could not unpack pprof profiles into map")
}
svc.profiles = profiles

go func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind moving these two lines to service.Start?

utils.Logger().Info().Str("address", cfg.ListenAddr).Msg("Starting pprof HTTP service")
utils.Logger().Info().Str("address", cfg.ListenAddr).Msg("starting pprof HTTP service")
http.ListenAndServe(cfg.ListenAddr, nil)
}()

Expand All @@ -93,8 +94,10 @@ func (s *Service) Start() error {
return err
}

if cpuProfile, ok := s.profiles[CPU]; ok {
go handleCpuProfile(cpuProfile, dir)
if _, ok := s.profiles[CPU]; ok {
// The nature of the pprof CPU profile is fundamentally different to the other profiles, because it streams output to a file during profiling.
// Thus it has to be started outside of the defined interval.
go restartCpuProfile(dir)
}

for _, profile := range s.profiles {
Expand All @@ -112,9 +115,12 @@ func (s *Service) Stop() error {
}
for _, profile := range s.profiles {
if profile.Name == CPU {
pprof.StopCPUProfile()
stopCpuProfile()
} else {
saveProfile(profile, dir)
err := saveProfile(profile, dir)
if err != nil {
utils.Logger().Error().Err(err).Msg(fmt.Sprintf("could not save pprof profile: %s", profile.Name))
}
}
}
return nil
Expand All @@ -135,9 +141,15 @@ func scheduleProfile(profile Profile, dir string) {
select {
case <-ticker.C:
if profile.Name == CPU {
handleCpuProfile(profile, dir)
err := restartCpuProfile(dir)
if err != nil {
utils.Logger().Error().Err(err).Msg("could not start pprof CPU profile")
}
} else {
saveProfile(profile, dir)
err := saveProfile(profile, dir)
if err != nil {
utils.Logger().Error().Err(err).Msg(fmt.Sprintf("could not save pprof profile: %s", profile.Name))
}
}
}
}
Expand All @@ -149,30 +161,39 @@ func scheduleProfile(profile Profile, dir string) {
func saveProfile(profile Profile, dir string) error {
f, err := newTempFile(dir, profile.Name, ".pb.gz")
JackyWYX marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
utils.Logger().Error().Err(err).Msg(fmt.Sprintf("Could not save pprof profile: %s", profile.Name))
return err
}
defer f.Close()
if profile.ProfileRef != nil {
err = profile.ProfileRef.WriteTo(f, profile.Debug)
if err != nil {
utils.Logger().Error().Err(err).Msg(fmt.Sprintf("Could not write pprof profile: %s", profile.Name))
return err
}
utils.Logger().Info().Msg(fmt.Sprintf("Saved pprof profile in: %s", f.Name()))
if profile.ProfileRef == nil {
return nil
}
err = profile.ProfileRef.WriteTo(f, profile.Debug)
if err != nil {
return err
}
utils.Logger().Info().Msg(fmt.Sprintf("saved pprof profile in: %s", f.Name()))
return nil
}

// handleCpuProfile handles the provided CPU profile
func handleCpuProfile(profile Profile, dir string) {
// restartCpuProfile stops the current CPU profile, if any and then starts a new CPU profile. While profiling in the background, the profile will be buffered and written to a file.
func restartCpuProfile(dir string) error {
stopCpuProfile()
f, err := newTempFile(dir, CPU, ".pb.gz")
if err != nil {
return err
}
pprof.StartCPUProfile(f)
cpuFile = f
utils.Logger().Info().Msg(fmt.Sprintf("saved pprof CPU profile in: %s", f.Name()))
return nil
}

// stopCpuProfile stops the current CPU profile, if any
func stopCpuProfile() {
pprof.StopCPUProfile()
f, err := newTempFile(dir, profile.Name, ".pb.gz")
if err == nil {
pprof.StartCPUProfile(f)
utils.Logger().Info().Msg(fmt.Sprintf("Saved pprof CPU profile in: %s", f.Name()))
} else {
utils.Logger().Error().Err(err).Msg("Could not start pprof CPU profile")
if cpuFile != nil {
Copy link
Contributor

@JackyWYX JackyWYX Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried testing with go test --race of this line? It seems there is a data race condition of cpuFile.

The race condition can be detected by

  1. implement a unit test with service.Start and last for at least one iteration.
  2. Test with go test --race.

cpuFile.Close()
cpuFile = nil
}
}

Expand All @@ -185,8 +206,8 @@ func (config *Config) unpackProfilesIntoMap() (map[string]Profile, error) {
for index, name := range config.ProfileNames {
profile := Profile{
Name: name,
Interval: 0,
Debug: 0,
Interval: 0, // 0 saves the profile when stopping the service
Debug: 0, // 0 writes the gzip-compressed protocol buffer
}
// Try set interval value
if len(config.ProfileIntervals) == len(config.ProfileNames) {
JackyWYX marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -203,7 +224,7 @@ func (config *Config) unpackProfilesIntoMap() (map[string]Profile, error) {
// Try set the profile reference
if profile.Name != CPU {
if p := pprof.Lookup(profile.Name); p == nil {
return nil, fmt.Errorf("Pprof profile does not exist: %s", profile.Name)
return nil, fmt.Errorf("pprof profile does not exist: %s", profile.Name)
} else {
niklr marked this conversation as resolved.
Show resolved Hide resolved
profile.ProfileRef = p
}
Expand All @@ -219,7 +240,7 @@ func newTempFile(dir, name, suffix string) (*os.File, error) {
currentTime := time.Now().Unix()
f, err := os.OpenFile(filepath.Join(dir, fmt.Sprintf("%s%d%s", prefix, currentTime, suffix)), os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
if err != nil {
return nil, fmt.Errorf("Could not create file of the form %s%d%s", prefix, currentTime, suffix)
return nil, fmt.Errorf("could not create file of the form %s%d%s", prefix, currentTime, suffix)
}
return f, nil
}
2 changes: 1 addition & 1 deletion api/service/pprof/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestUnpackProfilesIntoMap(t *testing.T) {
ProfileNames: []string{"test"},
},
expMap: nil,
expErr: errors.New("Pprof profile does not exist: test"),
expErr: errors.New("pprof profile does not exist: test"),
},
{
input: &Config{
Expand Down
6 changes: 3 additions & 3 deletions cmd/harmony/config_migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,13 @@ func init() {
confTree.Set("Pprof.Folder", defaultConfig.Pprof.Folder)
}
if confTree.Get("Pprof.ProfileNames") == nil {
confTree.Set("Pprof.ProfileNames", make([]interface{}, 0))
confTree.Set("Pprof.ProfileNames", defaultConfig.Pprof.ProfileNames)
}
if confTree.Get("Pprof.ProfileIntervals") == nil {
confTree.Set("Pprof.ProfileIntervals", make([]interface{}, 0))
confTree.Set("Pprof.ProfileIntervals", defaultConfig.Pprof.ProfileIntervals)
}
if confTree.Get("Pprof.ProfileDebugValues") == nil {
confTree.Set("Pprof.ProfileDebugValues", make([]interface{}, 0))
confTree.Set("Pprof.ProfileDebugValues", defaultConfig.Pprof.ProfileDebugValues)
}

confTree.Set("Version", "2.2.0")
Expand Down
4 changes: 2 additions & 2 deletions cmd/harmony/default.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ var defaultConfig = harmonyconfig.HarmonyConfig{
ListenAddr: "127.0.0.1:6060",
Folder: "./profiles",
ProfileNames: []string{},
ProfileIntervals: []int{},
ProfileDebugValues: []int{},
ProfileIntervals: []int{600},
ProfileDebugValues: []int{0},
},
Log: harmonyconfig.LogConfig{
Folder: "./latest",
Expand Down
2 changes: 1 addition & 1 deletion cmd/harmony/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ var (
}
pprofProfileDebugFlag = cli.IntSliceFlag{
Name: "pprof.profile.debug",
Usage: "a list of pprof profile debug integer values (separated by ,) e.g. 0 writes the gzip-compressed protocol buffer and 1 writes the legacy text format",
Usage: "a list of pprof profile debug integer values (separated by ,) e.g. 0 writes the gzip-compressed protocol buffer and 1 writes the legacy text format. Predefined profiles may assign meaning to other debug values: https://golang.org/pkg/runtime/pprof/",
DefValue: defaultConfig.Pprof.ProfileDebugValues,
Hidden: true,
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/harmony/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ func TestHarmonyFlags(t *testing.T) {
ListenAddr: "127.0.0.1:6060",
Folder: "./profiles",
ProfileNames: []string{},
ProfileIntervals: []int{},
ProfileDebugValues: []int{},
ProfileIntervals: []int{600},
ProfileDebugValues: []int{0},
},
Log: harmonyconfig.LogConfig{
Folder: "./latest",
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ require (
github.com/multiformats/go-multiaddr-dns v0.3.1
github.com/natefinch/lumberjack v2.0.0+incompatible
github.com/pborman/uuid v1.2.0
github.com/pelletier/go-toml v1.2.0
github.com/pelletier/go-toml v1.9.3
JackyWYX marked this conversation as resolved.
Show resolved Hide resolved
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.8.0
github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0
Expand Down