MonitoredMiddleware crashes if probe is not present

[Pixelshaped]

Following the Symfony Messenger configuration https://docs.blackfire.io/php/integrations/symfony/messenger

framework:
  messenger:
    buses:
      messenger.bus.default:
        middleware:
          - 'Blackfire\Bridge\Symfony\MonitoredMiddleware'

services:
  Blackfire\Bridge\Symfony\MonitoredMiddleware: ~

I've got a crash in MonitoredMiddleware at line 32: Class "BlackfireProbe" not found.

And it's true: on my local machine, I don't have the probe installed.

I fixed it by adding

when@prod:

So that MonitoredMiddleware only works on my prod env.

But I still think that if phpversion('blackfire') returns false, it should fail silently or log.

[thomasdiluccio]

I believe we should replace

if (version_compare(phpversion('blackfire'), '1.78.0', '>=')) {
 ....
}

with:

if (phpversion('blackfire') && version_compare(phpversion('blackfire'), '1.78.0', '>=')) {
 ....
}

to ensure the conditions fails if phpversion('blackfire') returns false;

[Pixelshaped]

Wouldn't be sufficient as the BlackfireProbeclass is used in both branches of the condition.

Something along those lines

<?php

/*
 * This file is part of the Blackfire SDK package.
 *
 * (c) Blackfire <support@blackfire.io>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Blackfire\Bridge\Symfony;

use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Middleware\MiddlewareInterface;
use Symfony\Component\Messenger\Middleware\StackInterface;
use Symfony\Component\Messenger\Stamp\ConsumedByWorkerStamp;

class MonitoredMiddleware implements MiddlewareInterface
{
    public function handle(Envelope $envelope, StackInterface $stack): Envelope
    {
        if (null === $envelope->last(ConsumedByWorkerStamp::class)) {
            return $stack->next()->handle($envelope, $stack);
        }

        $txName = \get_class($envelope->getMessage());

        $blackfireVersion = phpversion('blackfire');
        if ($blackfireVersion !== false){
            if (version_compare($blackfireVersion, '1.78.0', '>=')) {
                \BlackfireProbe::startTransaction($txName);
            } else {
                \BlackfireProbe::startTransaction();
                \BlackfireProbe::setTransactionName($txName);
            }
        }

        try {
            return $stack->next()->handle($envelope, $stack);
        } finally {
            if($blackfireVersion !== false) {
                \BlackfireProbe::stopTransaction();
            }
        }
    }
}

Would work.

[romainneutron]

Hello,

Silently failing is not an option. I prefer misconfiguration to explicitely fail so you can fix the configuration.
Using the Blackfire Middleware without the extension is unfortunately not possible, the only patch I could propose is an explicit RuntimeException:

diff --git a/languages/php/sdk/src/Blackfire/Bridge/Symfony/MonitoredMiddleware.php b/languages/php/sdk/src/Blackfire/Bridge/Symfony/MonitoredMiddleware.php
index cd58853e73..8fd10a1622 100644
--- a/languages/php/sdk/src/Blackfire/Bridge/Symfony/MonitoredMiddleware.php
+++ b/languages/php/sdk/src/Blackfire/Bridge/Symfony/MonitoredMiddleware.php
@@ -18,6 +18,13 @@ use Symfony\Component\Messenger\Stamp\ConsumedByWorkerStamp;
 
 class MonitoredMiddleware implements MiddlewareInterface
 {
+    public function __construct()
+    {
+        if (!extension_loaded('blackfire')) {
+            throw new \RuntimeException(sprintf('Extension "blackfire" is required to use "%s".', __CLASS__));
+        }
+    }
+
     public function handle(Envelope $envelope, StackInterface $stack): Envelope
     {
         if (null === $envelope->last(ConsumedByWorkerStamp::class)) {

Is it good for you ?

[Pixelshaped]

Not sure why silently failing is not an option.

As an example, when you implement the MonitorableCommandInterface on a Command (as per documentation https://docs.blackfire.io/php/integrations/symfony/cli-commands-monitoring) and you don't have the probe installed... nothing happens. The command runs and is not monitored and that's it.

How would you then justify this difference of behavior?

I think failing silently is a perfectly sane option in some cases.

If you want to throw an error though, it won't change much for me:

• Program will halt several lines earlier
• Message will be a little less cryptic than Class "BlackfireProbe" not found. but the previous one wasn't undecipherable either.
• Notification will not be handled properly

Not sure why the absence of the Blackfire extension should prevent this aspect of the program to run normally, while about any other aspect of the program runs fine in the absence of the probe.

One might even say that it's the default behavior of Blackfire to never report that it's correctly installed 😄

[romainneutron]

Hello,

Failing silently leads to situation where a user expects something that is not happening.
If a user explicitely uses a feature and it can not be used, it is expected to fail rather than do nothing silently.

Let's take an example: Imagine you're using Symfony, and depending on some environment configuration, some messages would not be dispatched by the event dispatcher, silently. There's high possibility it drives the user mad.

Here, it's the same kind of situation: you activated a feature, on purpose, in your configuration. But given your environment is missing an extension, it would not work. This would look like problematic for newcomers, they would not understand why it's not working.
If there's an explicit exception message, it's obvious.

Not sure why the absence of the Blackfire extension should prevent this aspect of the program to run normally, while about any other aspect of the program runs fine in the absence of the probe.

Because you explicitely activated the feature in your configuration.

As an example, when you implement the MonitorableCommandInterface on a Command (as per documentation https://docs.blackfire.io/php/integrations/symfony/cli-commands-monitoring) and you don't have the probe installed... nothing happens. The command runs and is not monitored and that's it.

That's correct, you're right. However we can not change the behavior without possibly breaking BC.

[Pixelshaped]

I get what you're saying, but this behavior is a bit of a discrepancy vs how Blackfire works usually which is failing silently on about every call when the extension is not installed (as illustrated by Commands or any http request).

Do what you must.

[sylfabre]

Thank you @Pixelshaped for this thread, it saved me some hours as I was investigating a configuration/CI failure issue due to this discrepancy in the SDK implementation.

ping FYI @romainneutron

[yariksheptykin]

Thanks @Pixelshaped for rising this issue. We were surprised to see all our AMQP messages landing in the failed queue just because of blackfire being unable to find the probe. Blackfire should not by design do anything, which could interrupt the usual flow of the application. No exceptions should be thrown. If you are concerned with warning the users about misconfiguration, do it using logs (bei injecting LoggerInterface) or provide a command for linting / validating the environment, which can be executed when needed. Please update the Middleware to perform a no-operation when no probe is detected and log a warning about misconfiguration.
The current implementation is rather surprising and unexpected that does not align with the rest of blackfire tooling and thus it should be treated as a bug. For people who rely on a this Bug the fix might indeed introduce a BC. Is there a problem in releasing a new major version?