Skip to content

feat: add profiling of Orchestrion#217

Merged
RomainMuller merged 4 commits into
mainfrom
nick.ripley/profile-orchestrion
Aug 14, 2024
Merged

feat: add profiling of Orchestrion#217
RomainMuller merged 4 commits into
mainfrom
nick.ripley/profile-orchestrion

Conversation

@nsrip-dd
Copy link
Copy Markdown
Contributor

@nsrip-dd nsrip-dd commented Aug 8, 2024

What does this PR do?

Add hidden flags to enable profiling Orchestrion. Options for writing
CPU and heap profiler, as well as execution traces, with a configurable
path prefix. Enabling the profiler for the parent enables it for the
children. There is one of each enabled profile type per process, and
each file gets the PID in its name.

Note that CPU profiling adds a ~200ms delay at the end of the app to
stop profiling. This might matter more for Orchestrion since there are
many normally short-lived processes.

Execution tracing should be enabled with caution since traces can be
quite large. Even the other profile types can produce significant
amounts of data. The profiles will have redundant symbol data. The CPU
and heap profiles can be combined into a single profile to eliminate the
redundancy.

Example usage:

$ mkdir profiles
$ orchestrion --profile-prefix=${PWD}/profiles --cpu-profiling=true go build
# To combine the profiles:
$ go tool pprof -proto profiles/*.pprof > cpu.pprof
# To view:
$ go tool pprof -http=localhost:6060 cpu.pprof

Motivation

Looking for hot spots & bottlenecks in Orchestrion

Reviewer's Checklist

  • Changed code has unit tests for its functionality.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 3.27869% with 59 lines in your changes missing coverage. Please review.

Project coverage is 60.79%. Comparing base (e5d6424) to head (c688347).
Report is 1 commits behind head on main.

Files Patch % Lines
main.go 3.27% 56 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
- Coverage   61.51%   60.79%   -0.73%     
==========================================
  Files          91       91              
  Lines        4841     4902      +61     
==========================================
+ Hits         2978     2980       +2     
- Misses       1537     1593      +56     
- Partials      326      329       +3     
Flag Coverage Δ
ARM64 43.83% <3.27%> (-0.72%) ⬇️
Linux 65.46% <3.27%> (-0.95%) ⬇️
Windows 42.72% <3.27%> (-0.57%) ⬇️
X64 60.79% <3.27%> (-0.73%) ⬇️
generator 42.64% <ø> (ø)
go1.21 52.39% <3.27%> (-0.67%) ⬇️
go1.22 50.22% <3.27%> (-0.60%) ⬇️
go1.23 49.00% <3.27%> (-0.58%) ⬇️
integration 45.36% <3.27%> (-0.61%) ⬇️
macOS 43.83% <3.27%> (-0.72%) ⬇️
unit 39.45% <0.00%> (-0.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add hidden flags to enable profiling Orchestrion. Options for writing
CPU and heap profiler, as well as execution traces, with a configurable
path prefix. Enabling the profiler for the parent enables it for the
children. There is one of each enabled profile type per process, and
each file gets the PID in its name.

Note that CPU profiling adds a ~200ms delay at the end of the app to
stop profiling. This might matter more for Orchestrion since there are
many normally short-lived processes.

Execution tracing should be enabled with caution since traces can be
quite large. Even the other profile types can produce significant
amounts of data. The profiles will have redundant symbol data. The CPU
and heap profiles can be combined into a single profile to eliminate the
redundancy.

Example usage:

    $ mkdir profiles
    $ orchestrion --profile-prefix=${PWD}/profiles --cpu-profiling=true go build
    # To combine the profiles:
    $ go tool pprof -proto profiles/*.pprof > cpu.pprof
    # To view:
    $ go tool pprof -http=localhost:6060 cpu.pprof
@nsrip-dd nsrip-dd force-pushed the nick.ripley/profile-orchestrion branch from 9a85353 to c688347 Compare August 9, 2024 12:35
@nsrip-dd nsrip-dd marked this pull request as ready for review August 9, 2024 12:36
@nsrip-dd nsrip-dd requested a review from a team as a code owner August 9, 2024 12:36
Comment thread main.go Outdated
Comment thread main.go Outdated
Comment thread main.go Outdated
Comment thread main.go Outdated
* Add "ORCHESTRION_" prefix to env vars.
* Use a single argument for enabling profiling, which can be repeated or
  given a comma-separated value to enable multiple profilers.
* Don't rely on the order of arg parsing. Enable profiles in the Before
  function, which runs after args are parsed.
* Changed profile-prefix to profile-path, and create the path if it
  doesn't exist.
* Don't store the profile path in a global var. Retrieve it in Before
  after parsing args and pass it to wherever its needed via function
  arguments.
Copy link
Copy Markdown
Contributor Author

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

Thanks for the review @RomainMuller. I made several changes based on your comments.

Comment thread main.go Outdated
Comment thread main.go Outdated
Comment thread main.go Outdated
@RomainMuller RomainMuller enabled auto-merge August 14, 2024 15:07
@RomainMuller RomainMuller added this pull request to the merge queue Aug 14, 2024
Merged via the queue into main with commit e5fd213 Aug 14, 2024
@RomainMuller RomainMuller deleted the nick.ripley/profile-orchestrion branch August 14, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants