Skip to content

feat: allow custom profilers for sampled transactions - #2253

Open
binaryfire wants to merge 2 commits into
getsentry:masterfrom
binaryfire:feat/profiler-factory
Open

binaryfire wants to merge 2 commits into
getsentry:masterfrom
binaryfire:feat/profiler-factory

Conversation

@binaryfire

Copy link
Copy Markdown
Contributor

This follows on from #2190 and #2191, this time for profiling.

Excimer records whichever PHP stack is running when it takes a sample. In an async server, that might belong to a different request. For example, while request A is waiting for I/O, request B can run and have its work included in A’s profile.

An integration can use Excimer’s existing callbacks to separate those samples, but the SDK creates and manages its profiler internally. There isn’t currently a supported way to replace how samples are collected.

This PR adds a profiler_factory option:

Sentry\init([
    'traces_sample_rate' => 1.0,
    'profiles_sample_rate' => 0.1,
    'profiler_factory' => static function (Options $options): ProfilerInterface {
        return new RequestProfiler($options);
    },
]);

Here, RequestProfiler would be supplied by the framework or application. The factory creates a separate profiler for each sampled transaction. The interface has three methods: start(), stop(), and getProfile().

Sentry still makes the sampling decisions and handles formatting and sending the resulting profile. The integration controls how samples are collected and when its collection resources are released.

Without a factory, the SDK continues to use its existing Excimer profiler. The factory is only called for transactions selected for profiling; this doesn’t add a callback to each stack sample.

Finishing a transaction twice also stops its profiler only once. Previously, the second stop could replace the collected samples with an empty log before the event was serialized.

I tested this with a custom collector using stock Excimer and PHP Fibers. The SDK produced a valid profile containing the intended Fiber’s work and excluding work performed while it was suspended. No Excimer changes were needed.

I ran into this while working on Hypervel, but the extension isn’t tied to it or to Swoole. Other async frameworks can use their own execution context to decide which samples belong to a transaction.

The options documentation will also need an entry for profiler_factory.

Expose the existing start, stop and profile retrieval operations through ProfilerInterface so integrations can supply their own sample collectors.

Keep the Excimer implementation unchanged and make the Profile construction and sample input methods public contracts. Document the sample timestamp base while keeping envelope formatting internal.
Allow integrations to create an independent sample collector for each profiled transaction through profiler_factory. Retain the default Excimer profiler and the existing trace and profile sampling decisions.

Log collector failures without losing the transaction, and stop a collector only once when finish is called repeatedly so pending profile data remains intact.

Validate options, factory ownership, profile forwarding and failure handling. composer check passes with 1998 tests, 5050 assertions and 11 skips; a stock Excimer and Fiber integration probe also produces a valid profile without unrelated execution samples.
@binaryfire

Copy link
Copy Markdown
Contributor Author

I've added the companion docs in getsentry/sentry-docs#19890. It's a draft until the SDK change is released.

@Litarnus Litarnus 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.

Looks very nice, thank you!

We recently added try/catch around user provided callbacks, could you please add it here too? It doesn't have to be anything fancy, a log message is enough

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