-
Notifications
You must be signed in to change notification settings - Fork 165
[Bug]: Symfony Command "sh" shell_exec error spans #3127
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
Comments
So, if I understand it right, the goal is to specifically not trace that specific span, right?
Untested - it should prevent the span from being created at all during that function. Something along these lines should suppress the span creation. This is a bit hacky though and I agree that there should be a better mechanism to selectively suppress spans from integrations. Thinking that possibly disabling the integration via ini should temporarily pause the spans from that integration (and resume once re-enabled again). |
@bwoebi Yes that's right. We only want to get rid of that specific span. I can give that a try. I also thought about disabling the integration. If possible, we would like to keep the exec integration, to have visibility into the useful things it tracks. |
hook_method will also work, I'm mostly using install_hook to stuff the hook id into the $data property to remove_hook it afterwards. But suppressCall is specific to install_hook. |
Yes that makes sense. I'll try it that way |
@bwoebi I adapted your code a bit, since it would basically have the same effect as completely disabling the exec integration, which we want to use for all other cases. I got it to work by calling the following method. /**
* Remove "sh" span added by the "shell_exec" hook created by {@see \DDTrace\Integrations\Exec\ExecIntegration}.
* Because the {@see Symfony\Component\Console\Terminal::hasSttyAvailable()} method runs a shell command (stty 2> /dev/null)
* that always returns null if no TTY is available (which is always the case on the server).
* The "shell_exec" hook of the ExecIntegration treats null return values as errors.
* Thus, an error span with the name "sh" is always added to commands that print something.
* This causes issues with trace ingestion sampling.
* Since these "shell_exec" spans are errors, the error sampler is triggered (https://docs.datadoghq.com/tracing/trace_pipeline/ingestion_mechanisms/?tab=php#error-traces).
* Which circumvents our sampling rate config.
* And manually dropping/ignoring the "sh" resource does not work because we use DD_APM_FEATURES=error_rare_sample_tracer_drop,
* because we want that all error traces are sampled, regardless of the sample rate of the resource.
*/
private function preventSymfonyConsoleShellExecErrorSpan(): void
{
\DDTrace\install_hook('Symfony\Component\Console\Application::doRun', static function (HookData $hook): void {
$hook->data = \DDTrace\install_hook('DDTrace\Integrations\Exec\ExecIntegration::createSpan', static function (HookData $hook): void {
// We call this because we use suppressCall()
$hook->disableJitInlining();
if (
isset($hook->args[0][Tag::EXEC_CMDLINE_SHELL])
&& $hook->args[0][Tag::EXEC_CMDLINE_SHELL] === 'stty 2> /dev/null'
) {
$hook->suppressCall();
}
});
}, static function (HookData $hook): void {
// The hook on ExecIntegration::createSpan does not have to exist anymore once command execution is finished
\DDTrace\remove_hook($hook->data);
});
} |
@orlandothoeny Ah, I see, I did not check what exactly the method is here :-) It encompasses everything, so yeah. But in that case wouldn't it be simpler to just hook Anyway, I'll look at enhancing this soon in our symfony integration too. This particular span is pure noise :-) |
@bwoebi You mean just replacing Cool, would be nice if the Symfony integration handled this itself. |
It does make the code a little bit shorter :) \DDTrace\install_hook('Symfony\\Component\\Console\\Terminal::hasSttyAvailable', static function (
DDTrace\HookData $hook,
): void {
\DDTrace\install_hook('DDTrace\\Integrations\\Exec\\ExecIntegration::createSpan', static function (
DDTrace\HookData $hook,
): void {
$hook->disableJitInlining();
$hook->suppressCall();
\DDTrace\remove_hook($hook->id);
});
}); |
Ah yes, I see what you mean |
Uh oh!
There was an error while loading. Please reload this page.
Bug report
We experience the same issue as described in #2589.
But I cannot reopen that bug, so I'm creating a new one.
We have error spans with
operation=command_execution
andresource=sh
in our Symfony Command traces. These would be no issue, if we could just remove them with a sampling rate of 0 using tracer sampling rules. But that does not work, because we use theDD_APM_FEATURES=error_rare_sample_tracer_drop
setting (Docs).If I understand the documentation correctly, we would loose errors of traces that are removed by our custom dd-trace sampling rules if we disable the
error_rare_sample_tracer_drop
setting:So we cannot remove these
sh
traces at the moment.I had a look if we can remove these
sh
spans manually. But as I understand, this is not possible. I'm not 100% sure though.One thing I saw was the
\dd_untrace('shell_exec');
function, but as I understand, this will just remove registered hooks. The code that adds thesh
traces (ExecIntegration::init()) however, adds a hook that manually creates a span. So I'm not sure if usingdd_untrace()
would work.Creating a posthook around
shell_exec
using\DDTrace\hook_method()
and just removing all the error metadata from thesh
span also does not work I think. I don't see a way how we could gain access to the span created by theExecIntegration
hook.Here's some pseudocode of these thoughts:
Example sampling rule config (from
php --ri=ddtrace; php --ri=datadog-profiling
):The code that runs the
stty 2> /dev/null
shell command is the following: https://github.com/symfony/console/blob/e51498ea18570c062e7df29d05a7003585b19b88/Terminal.php#L131This code is triggered when a Symfony Command runs.
We are using Symfony 5.4, so would be great if a fix would also cover that version if it is somehow tied to Symfony.
PHP version
8.2.27
Tracer or profiler version
0.99.1
Installed extensions
Output of
phpinfo()
Upgrading from
No response
The text was updated successfully, but these errors were encountered: