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

feat: add continuous profiler instrumentation #178

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented Jul 22, 2024

PROF-10258

What does this PR do?

Add profiling, enabled manually by setting DD_PROFILING_ENABLED=true at run time.
The profiler has roughly the default configuration, with a few non-default profile types which
should be low-enough overhead and sufficiently useful to enable for recent Go releases.
There are not currently any other configuration knobs.

If enabled this way, the profiles will be tagged with orchestrion:true.

This PR also accepts the value DD_PROFILING_ENABLED=auto to enable profiling.
This can be provided via the Datadog Admission Controller (see this PR).
Some languages/runtimes use heuristics to decide whether to enable profiling when
auto is provided, to avoid profiling short-lived or non-instrumented applications.
We assume the application is meant to be instrumented by virtue of the user building
with Orchestrion. And the Go profiler won't send any data until at least one minute has
passed. So, we treat auto the same as true.

TODO - document this once it's released

TODO - this has only been manually tested. That's probably okay for now; there's not
much to this code. But we can investigate ways to automatically test it.

Motivation

If somebody uses Orchestrion to add APM instrumentation, and they also want
profiling, Orchestrion should be able to add it so the user doesn't have to
separately modify their code to get profiling.

Reviewer's Checklist

  • Changed code has unit tests for its functionality.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.08%. Comparing base (745a9ef) to head (7d7c156).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #178   +/-   ##
=======================================
  Coverage   65.08%   65.08%           
=======================================
  Files          76       76           
  Lines        3993     3993           
=======================================
  Hits         2599     2599           
  Misses       1124     1124           
  Partials      270      270           
Flag Coverage Δ
ARM64 47.58% <ø> (ø)
Linux 69.54% <ø> (ø)
Windows 46.47% <ø> (ø)
X64 65.08% <ø> (ø)
generator 42.87% <ø> (ø)
go1.21 55.51% <ø> (ø)
go1.22 52.11% <ø> (ø)
go1.23 51.84% <ø> (ø)
integration 46.88% <ø> (ø)
macOS 47.58% <ø> (ø)
unit 40.50% <ø> (+0.78%) ⬆️

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.

@nsrip-dd nsrip-dd force-pushed the nick.ripley/add-profiling branch 3 times, most recently from 2291890 to 7a1483b Compare July 29, 2024 18:40
Add continuous profiling to instrumented applications. The profiler gets
basically the default configuration, and is activated at run time by
setting the environment variable DD_PROFILING_ENABLED to any of "1",
"true", or "auto".
@nsrip-dd nsrip-dd force-pushed the nick.ripley/add-profiling branch from 7a1483b to da7d13d Compare July 30, 2024 15:39
@nsrip-dd nsrip-dd changed the title WIP: add profiling feat: add continuous profiler instrumentation Jul 30, 2024
@nsrip-dd nsrip-dd marked this pull request as ready for review July 30, 2024 16:41
@nsrip-dd nsrip-dd requested a review from felixge July 30, 2024 16:41
@RomainMuller RomainMuller enabled auto-merge July 31, 2024 09:31
Signed-off-by: github-actions on behalf of RomainMuller <[email protected]>
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

Thank you so much 🙇 . This LGTM.

@nsrip-dd nsrip-dd requested a review from a team as a code owner August 6, 2024 15:06
@RomainMuller RomainMuller added this pull request to the merge queue Aug 6, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 6, 2024
PROF-10258

### What does this PR do?

Add profiling, enabled manually by setting `DD_PROFILING_ENABLED=true`
at run time.
The profiler has roughly the default configuration, with a few
non-default profile types which
should be low-enough overhead and sufficiently useful to enable for
recent Go releases.
There are not currently any other configuration knobs.

If enabled this way, the profiles will be tagged with
`orchestrion:true`.

This PR also accepts the value `DD_PROFILING_ENABLED=auto` to enable
profiling.
This can be provided via the [Datadog Admission
Controller](https://docs.datadoghq.com/containers/cluster_agent/admission_controller)
(see [this PR](DataDog/datadog-agent#27185)).
Some languages/runtimes use heuristics to decide whether to enable
profiling when
`auto` is provided, to avoid profiling short-lived or non-instrumented
applications.
We assume the application is meant to be instrumented by virtue of the
user building
with Orchestrion. And the Go profiler won't send any data until at least
one minute has
passed. So, we treat `auto` the same as `true`.

TODO - document this once it's released

TODO - this has only been manually tested. That's probably okay for now;
there's not
much to this code. But we can investigate ways to automatically test it.

### Motivation

If somebody uses Orchestrion to add APM instrumentation, and they also
want
profiling, Orchestrion should be able to add it so the user doesn't have
to
separately modify their code to get profiling.

### Reviewer's Checklist
<!--
* Authors can use this list as a reference to ensure that there are no
problems
during the review but the signing off is to be done by the reviewer(s).
-->

- [ ] Changed code has unit tests for its functionality.

---------

Signed-off-by: github-actions on behalf of RomainMuller <[email protected]>
Co-authored-by: Romain Marcadier <[email protected]>
Co-authored-by: github-actions on behalf of RomainMuller <[email protected]>
Co-authored-by: Romain Marcadier <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 7, 2024
@RomainMuller RomainMuller added this pull request to the merge queue Aug 7, 2024
Merged via the queue into main with commit 4a6f7eb Aug 7, 2024
24 checks passed
@RomainMuller RomainMuller deleted the nick.ripley/add-profiling branch August 7, 2024 09:06
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.

4 participants