Add profiling sources by morrisonlevi · Pull Request #1606 · DataDog/dd-trace-php

morrisonlevi · 2022-06-08T03:08:30Z

Description

This PR does not:

Alter the tracer in any way, including its build requirements.
Add new tests to CI, and will only fix/adjust lint jobs if necessary so that existing tracer jobs will not be affected by the profiling subdirectory.
Build out of the box with cargo build. You have to check out Datadog/libdatadog's main branch to profiling/libdatadog. I didn't want to add a git submodule without discussion and the crates for libdatadog are not yet published, so this is a compromise. Edit: libdatadog v0.7.0 has been released on github since then, so we can probably do something like libdatadog = { git = "https://github.com/DataDog/libdatadog", tag = "v0.7.0" }. I'll look into this soon.

This PR does:

Add sources for a PHP profiler written almost entirely in Rust. It is essentially a port of the existing C profiler found at Datadog/dd-prof-php.
Have some tests, but not many. As mentioned above, they are not run in CI.

Fixing these things shortcomings will likely come in further PRs, as this one is already at +2,481 additions and I expect it will take some time to be able to get it reviewed and merged, especially as it needs reviewed by both Rust and PHP experts who are busy working on their own things.

There are a variety of todo tasks that I do not expect to be handled in this PR, like unifying components/sapi with profiling/src/sapi.rs. However, there is one about zend_bool that I still need to audit. Given that I expect this PR to take a while to get fully reviewed and that it hasn't caused observable issues yet, I decided to open the PR and do this audit concurrently (and hopefully take no action except remove the todo).

Readiness checklist

Changelog has been added to the release document.
~~Tests added for this feature/bug.~~

Reviewer checklist

Appropriate labels assigned.
Milestone is set.

Changelog from v0.7

Add process_id and runtime_version tags.
Add support for changing env vars per request, such as per-directory env var settings in Apache.
Improve logging. Added a new log level trace, which is even more verbose than debug.
Stop sending a profile on every phpinfo() (or the equivalent command line option --ri datadog-profiling).

bruceg

I don't have enough knowledge of the broader context to comment on the overall approach, so this review just reflects on the smaller details.

blt · 2022-06-09T22:35:17Z

+    }
+
+    pub fn stop(self) {
+        // todo: what should be done when a thread panics?


Intentionally left TODO?

Yes, but also I really don't know what ought to be done. In general, I don't think anything can be done except to propagate the panic.

FWIW this is a Hard™ problem, and we build Mork with panic=abort for that reason.

panic=abort seems nice, but to really get it right you need the stdlib to be rebuilt with it. I couldn't get that to work on Alpine, at the very least, though I haven't tried the latest Alpine release.

mokomull

I didn't really get a chance to look too hard at profiling.rs, but a few comments nonetheless. Mostly style and "a crate already exists to do that" flavoured suggestions.

mokomull · 2022-06-10T18:26:18Z

+// todo: this a lie on some PHP versions; is it a problem even though zend_bool
+//       was always supposed to be 0 or 1 anyway?


Perhaps: "A value other than false (0) or true (1) in a bool." (https://doc.rust-lang.org/beta/reference/behavior-considered-undefined.html)

mokomull · 2022-06-13T23:48:14Z

+        .output()
+        .expect("Unable to run `php-config`. Is it in your PATH?");
+
+    let prefix = String::from_utf8(output.stdout).expect("only utf8 chars work");


If you really wanted to support that you could use Path instead of Strings, but I'm pretty sure any user that's done that has already broken every ./configure script on their system :)

I don't remember the details, but I actually tried to use Path and couldn't get all the plumbing I needed to work out. For instance, how do I convert the bytes from the output of php-config into a Path?

mokomull · 2022-06-14T00:45:41Z

+    // The strings will be interned but potentially only for the request, so be
+    // careful to not use them outside of a request (such as from other
+    // threads).
+    datadog_php_str env;
+    datadog_php_str service;
+    datadog_php_str version;
+    datadog_php_str trace_agent_host;
+    datadog_php_str trace_agent_port;
+    datadog_php_str trace_agent_url;


Can these change from request-to-request (particularly wondering about sapi_getenv() ... the libc getenv() shouldn't really change during execution).

If not, then perhaps we should hold a void* to a Box::into_raw() of a Rust structure so we don't have to bounce these strings through FFI so much just for them to end back up in a Vec<Tag>

They can change from request to request, yes. For instance, in Apache you can configure env vars to have different values for different directories, think DD_SERVICE=myservice but in /admin it's DD_SERVICE=myservice-admin or something.

The code is partly set up the way it is because PHP's request globals have support for being tied to .ini configuration. I had previously tried to add .ini support, but hit an issue with certain configs like datadog.service being shared by the tracer and profiler, and the engine does not like this (it's not a C vs Rust issue, but a PHP extension issue). So, I pulled out all of the directly dead code but some of the influence is still there. I think I need to support .ini configuration so I'm not sure it's worth making the Rust side of things cleaner at the moment since I'm highly likely to undo that work for .ini. Dunno, I'll think about it.

mokomull · 2022-06-14T00:48:09Z

+
+#[derive(Debug, Clone)]
+pub enum LabelValue {
+    Str(Cow<'static, str>),


This seems to only ever be used for format!() so perhaps just take a String for simplicity?

mokomull · 2022-06-14T00:49:30Z

+    }
+
+    pub fn stop(self) {
+        // todo: what should be done when a thread panics?


FWIW this is a Hard™ problem, and we build Mork with panic=abort for that reason.

mokomull · 2022-06-14T00:50:52Z

+        static mut SAPIS: MaybeUninit<HashMap<&str, Sapi>> = MaybeUninit::uninit();
+        static ONCE: Once = Once::new();
+        ONCE.call_once(|| {
+            let sapis = HashMap::from([
+                ("apache2handler", Sapi::Apache2Handler),
+                ("cgi-fcgi", Sapi::CgiFcgi),
+                ("cli", Sapi::Cli),
+                ("cli-server", Sapi::CliServer),
+                ("embed", Sapi::Embed),
+                ("fpm-fcgi", Sapi::FpmFcgi),
+                ("litespeed", Sapi::Litespeed),
+                ("phpdbg", Sapi::PhpDbg),
+                ("tea", Sapi::Tea),
+            ]);
+            unsafe { SAPIS.write(sapis) };
+        });


This seems like a job for the maplit crate.

Thanks, I'll take a look at maplit.

I think before Rust 1.56 that maplit would be nice, but since then we have from and from_iter for HashMap. Using maplit would be changing out the (_, _), structure for _ => _, via macro, and I'm not sure how that's better? If it could do it in const contexts that would be awesome, but it can't. Am I missing something?

They won't do anything

Also cleanup some type usage while I'm at it.

morrisonlevi · 2022-07-07T02:49:22Z

Thanks @mokomull, @blt , and @bruceg. I've addressed most of the feedback, though I'm still thinking about some of them like changing the globals struct.

morrisonlevi · 2022-07-08T16:48:22Z

Added a CODEOWNERS file as requested, but it's currently invalid as DataDog/profiling-php doesn't yet have write access.

bwoebi

I assume that before a GA, we will still see proper ini handling.

In general the code reads pretty clean. Looks good to me for initial inclusion.

This reverts commit 3cc49a9. This was only a temporary measure to not waste CI cycles.

morrisonlevi added 2 commits June 7, 2022 17:16

Add profiling sources

688eba5

Merge branch 'master' into levi/profiling-rs

ffe9011

bruceg reviewed Jun 8, 2022

View reviewed changes

Simplify code, address CR feedback

1172553

blt reviewed Jun 9, 2022

View reviewed changes

mokomull reviewed Jun 14, 2022

View reviewed changes

morrisonlevi added 6 commits June 24, 2022 19:25

Address some CR feedback

d0b7ec9

Fix profiler and sapi

d559a29

Make RUNTIME_ID safe and document many unsafes

39bc29b

Fix sapi tag name

c428378

Bump version

849b59b

Avoid running circleci jobs on this branch

3cc49a9

They won't do anything

morrisonlevi force-pushed the levi/profiling-rs branch from e03cc07 to 3cc49a9 Compare July 6, 2022 15:10

morrisonlevi added 11 commits July 6, 2022 09:30

Adjust C style

e0b68b9

Document why we duplicate stderr.

6d93292

Remove dead code

fb59fb7

Remove dead code

e9e32fe

Move generated bindings to a sub-module.

7a3fd3a

Also cleanup some type usage while I'm at it.

Support older CPU Time env var

40777f9

Fix clippy lint

18a74f5

Document get_module

8ab9468

Document global profiler

3940e56

Remove redundant profile.dev settings

a9c88b9

Address more PR feedback

98ad9bd

morrisonlevi added 5 commits July 7, 2022 15:47

Sync version with cargo package; add profiler_version tag

0601283

Leak module entry instead of unsafe code

684d3e6

Merge branch 'master' into levi/profiling-rs

a5dc736

Depend on libdatadog from git tag

a5da988

Add CODEOWNERS

aab4ff2

Add apm-php to everything else

19e3da9

bwoebi previously approved these changes Jul 13, 2022

View reviewed changes

Revert "Avoid running circleci jobs on this branch"

17561fa

This reverts commit 3cc49a9. This was only a temporary measure to not waste CI cycles.

morrisonlevi dismissed bwoebi’s stale review via 17561fa July 18, 2022 14:30

bwoebi approved these changes Jul 18, 2022

View reviewed changes

morrisonlevi merged commit 1ee5916 into master Jul 18, 2022

morrisonlevi deleted the levi/profiling-rs branch July 18, 2022 17:00

morrisonlevi added this to the 0.77.0 milestone Jul 18, 2022

morrisonlevi added the profiling Relates to the Continuous Profiler label Jul 18, 2022

morrisonlevi mentioned this pull request Aug 2, 2022

Release 0.77.0-beta.1 #1684

Closed

		// todo: this a lie on some PHP versions; is it a problem even though zend_bool
		// was always supposed to be 0 or 1 anyway?

Uh oh!

Conversation

morrisonlevi commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Readiness checklist

Reviewer checklist

Changelog from v0.7

Uh oh!

bruceg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokomull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morrisonlevi commented Jul 7, 2022

Uh oh!

morrisonlevi commented Jul 8, 2022

Uh oh!

bwoebi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

morrisonlevi commented Jun 8, 2022 •

edited

Loading