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

Extra check for the newrelic extension #221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carlcasbolt
Copy link

I've discovered that I cannot have monolog.enabled = true without the newrelic extension installed because monolog's NewRelicHandler will throw a MissingExtensionException.

Is there a better way of achieving this?

Locally this means I can use the same yaml file from a machine with the extension installed and not see the above exception

I've discovered that I cannot have `monolog.enabled = true` without the newrelic extension installed because monolog's NewRelicHandler will throw a `MissingExtensionException`.

Is there a better way of achieving this?

Locally this means I can use the same yaml file from a machine with the extension installed and not see the above exception
@@ -120,7 +120,7 @@ public function load(array $configs, ContainerBuilder $container): void
$loader->load('twig.xml');
}

if ($config['enabled'] && $config['monolog']['enabled']) {
if (\extension_loaded('newrelic') && $config['enabled'] && $config['monolog']['enabled']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is done in Configuration.php already, isnt it?
Or that is only if you use: enable: auto?

Copy link
Author

Choose a reason for hiding this comment

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

enabled needs to be a boolean or you get this error:

Invalid type for path "ekino_new_relic.enabled". Expected boolean, but got string.

Copy link
Author

Choose a reason for hiding this comment

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

When the value is set to true, but the newrelic bundle is not installed you get this errors:

The newrelic PHP extension is required to use the NewRelicHandler

Which is thrown within the Monolog NewRelicHandler:

    protected function write(array $record)
    {
        if (!$this->isNewRelicEnabled()) {
            throw new MissingExtensionException('The newrelic PHP extension is required to use the NewRelicHandler');
        }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like an expected behavior. If you enable it, but don't have the extension, it'll faill.

Like using doctrine with pgsql DSN without the extension.

Copy link
Author

Choose a reason for hiding this comment

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

That's a fair point, I guess the issue is having the same yaml file for machines with the extension installed and then local machines without it.

What I should do it use an .env value to default to false locally and then true on deployed installs.

@carlcasbolt
Copy link
Author

Is there any update on this @Nyholm

@Nyholm
Copy link
Collaborator

Nyholm commented Jun 28, 2019

I vote for closing this issue and deprecating the “enabled” option.

Here is the config I was referring to: https://github.com/ekino/EkinoNewRelicBundle/blob/master/README.md#interactor-services

@carlcasbolt
Copy link
Author

I agree, that maybe this enabled option should be deprecated to avoid future confusion and rely on the extension being installed.

@Nyholm
Copy link
Collaborator

Nyholm commented Jun 28, 2019

You should set the enable: true and use the adaptiveInteractor. That should work for you for all your machines.

@PWalkow
Copy link

PWalkow commented Nov 27, 2019

Hi, unfortunately I have not found the way how to make it work either,

our first staging environment is using dev mode, not prod due to need of profiler bar, we have working set up for this instance within new-relic so we could find errors (from LoggerInterface->error('zzz'); on new-relic as the box is having the extension present. It works there but can't make it work on local for dev environment without playing with the configs manually

minimum configuration to start failing locally (without extension loaded):

### composer.json with
"ekino/newrelic-bundle": "^2.1",
"symfony/monolog-bundle": "^3.5",
### ekino_new_relic.yaml
ekino_new_relic:
    enabled: true
    application_name: 'My Symfony Application' # Remove this to use the value from php.ini
    api_key: "%env(NEWRELIC_API_KEY)%"
    exceptions: true
    deprecations: true
    monolog:
        enabled: true
        level: error

Interactor is set to the adaptiveInteractor as default obviously.

Then:

### AnyController
function someRoute(LoggerInterface $logger) {
      $logger->error('this should just log me a message');
}

would throw MissingExtensionException (The newrelic PHP extension is required to use the NewRelicHandler) in
vendor/monolog/monolog/src/Monolog/Handler/NewRelicHandler.php (line 78)

Maybe the same logic should be applied to monolog setup as interactive adapter has currently, to not set monolog handler if extension is not present within

###EkinoNewRelicExtension.php:123
if ($config['enabled'] && $config['monolog']['enabled']) {
   if (!\class_exists(\Monolog\Handler\NewRelicHandler::class)) {
      throw new \LogicException('The "symfony/monolog-bundle" package must be installed in order to use "monolog" option.');
   }
   $loader->load('monolog.xml');
   $container->setParameter('ekino.new_relic.monolog', $config['monolog'] ?? []);
   $container->setParameter('ekino.new_relic.application_name', $config['application_name']);
   $container->setAlias('ekino.new_relic.logs_handler', $config['monolog']['service'])->setPublic(false);
}

?

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