Skip to content

toolkit: add feature to profile & trace to have better diagnostics#5640

Merged
mfrw merged 8 commits into
mainfrom
mfrw/trace-and-pprof
Jun 27, 2023
Merged

toolkit: add feature to profile & trace to have better diagnostics#5640
mfrw merged 8 commits into
mainfrom
mfrw/trace-and-pprof

Conversation

@mfrw

@mfrw mfrw commented Jun 7, 2023

Copy link
Copy Markdown
Member

Signed-off-by: Muhammad Falak R Wani falakreyaz@gmail.com

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

What does the PR accomplish, why was it needed?
Add profiling and tracing support to the toolkit. The go toolchain has inbuilt tools to diagnose logic and performance problems in Go programs. This PR integrates that support for our toolkit.

This PR introduces 3 additional flags, by default all the three flags are disabled.

  • ENABLE_TRACE: Setting this flag to y, instruments the various tools in the toolkit to record trace data. Tracing is a way to instrument code to analyze latency throughout the lifecycle of a call or user request. Traces provide an overview of how much latency each component contributes to the overall latency in a system. Traces can span multiple Go processes.
  • ENABLE_MEM_PROFILE: Setting this flag to y instruments the toolkit to record the memory profile.
  • ENABLE_CPU_PROFILE: Setting this flag to y instruments the toolkit to record the cpu profile.

Profiling tools analyze the complexity and costs of a Go program such as its memory usage and frequently called functions to identify the expensive sections of a Go program.

It is advisable to only turn one of the flags for a run to get data that does not have noise due to other profiling/tracing flags.

The trace/profile data will be created in the build/profile directory.
To use the profile data: go tool pprof -http=:8080 file.profile
To use the trace data: go tool trace -http=:8080 file.trace

Change Log
  • NA
Does this affect the toolchain?

NO

Associated issues
  • NA
Links to CVEs
  • NA
Test Methodology
  • Local Build

@microsoft-github-policy-service microsoft-github-policy-service Bot added the main PR Destined for main label Jun 7, 2023
@mfrw mfrw changed the title toolkit: add feature to profile & trace to gain better observability toolkit: add feature to profile & trace to have better diagnostics Jun 7, 2023
@mfrw mfrw marked this pull request as ready for review June 7, 2023 10:19
@mfrw mfrw requested a review from a team as a code owner June 7, 2023 10:19
Comment thread toolkit/tools/grapher/grapher.go Outdated
Comment thread toolkit/scripts/pkggen.mk
@mfrw mfrw force-pushed the mfrw/trace-and-pprof branch from c025414 to 09487d4 Compare June 8, 2023 06:56
@mfrw mfrw requested a review from dmcilvaney June 9, 2023 02:53
@mfrw mfrw force-pushed the mfrw/trace-and-pprof branch 3 times, most recently from 37feff7 to cb4e4d0 Compare June 13, 2023 02:01
Comment thread toolkit/tools/grapher/grapher.go Outdated
Comment thread toolkit/tools/pkg/profile/profile.go Outdated
@mfrw mfrw force-pushed the mfrw/trace-and-pprof branch from cb4e4d0 to fdcda8c Compare June 19, 2023 10:01
@mfrw mfrw requested a review from dmcilvaney June 19, 2023 10:02
Comment thread toolkit/tools/pkg/profile/profile.go Outdated
mfrw added 6 commits June 21, 2023 09:24
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
@mfrw mfrw force-pushed the mfrw/trace-and-pprof branch from 1780daa to 5da038e Compare June 21, 2023 03:54
@mfrw mfrw requested a review from dmcilvaney June 21, 2023 03:56
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
if *pf.EnableCpuProf {
cpf, err := os.Create(*pf.CpuProfFile)
if err != nil {
return nil, fmt.Errorf("Unable to create cpu-pprof file: %s", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Linter will probably be grumpy about the capitalization of "Unable". It likes error strings to be all lowercase.

Comment thread toolkit/tools/grapher/grapher.go

@dmcilvaney dmcilvaney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The big block of flags in the Makefile still seems messy, but not a deal breaker.
Added one suggestion to make the error 'optional' but I think this looks ok.

Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
@mfrw mfrw merged commit 5f89ff9 into main Jun 27, 2023
@mfrw mfrw deleted the mfrw/trace-and-pprof branch June 27, 2023 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

main PR Destined for main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants