Raven logger can corrupt module_implements_alter when instantiated before DrupalKernel has loaded module files

Given that this seems to be a race condition while warming caches, it would be ideal for core to figure out a fix, if possible :)

Meanwhile, I guess the easiest workaround would be for Raven to migrate from hooks to events, but of course that's a BC break :/

The only reason we use hooks is because this module was originally developed back in Drupal 7 days - in retrospect I should have done this migration already, but better late than never.

By the way, my stance is that Raven module needs to initialize itself as soon as possible so that custom code is able to capture events or call other \Sentry functions as soon as possible, and fatal errors can be logged. Because of early bootstrap issues such as this one, there is no Raven middleware, but if a middleware initializes the loggers then boom, we hit those same issues.

Looks like the underlying issue is similar to that raised in #3134074: Avoid complex logic in constructor → , which was closed working as designed. Worth a look if only to see the discussion @ericgsmith.

Here's a patch to finally migrate client options from alter hook to event. Maybe you could verify that enabling this new setting (i.e. disabling the hook) resolves the issue?

I'm also thinking we could deprecate the filter and breadcrumb alter hooks, because these are made redundant by the before_send and before_breadcrumb callbacks, as well as the ability to add arbitrary integrations / event processors.

Re: #3134074: Avoid complex logic in constructor → I'd love to figure out a way to lazily initialize Sentry if/when needed for better performance. But the general concept of Sentry is that it's initialized early on so fatal errors can be captured and developers are free to sprinkle their code with constructs like

 \Sentry\configureScope(function (\Sentry\State\Scope $scope): void {
    $scope->setContext('Starfleet', [
        'name' => 'Spock',
        'rank' => 'Captain',
        'serial' => 'S179-276SP',
    ]);
}); 

If a developer wrote such code but Sentry was only initialized later, their code would have no effect. So it seemed vaguely reasonable to assume that if/when the logger is initialized, then developers could assume Sentry is also initialized, and to have a request event subscriber that initializes the logger in case no other code does.

The last submitted patch, 6: 3354760.patch, failed testing. View results →

Let's try again, patch was missing a file.

Fixing typo in README

Fixing more cases of the same typo :p

Pass a reference to the options array, otherwise it cannot be altered

@mfb thank you for the quick response!

The patch looks good - I've test it with config for set disable_deprecated_options_alter to true and I was no longer able to produce the error I previously was on a standard request.

Re filter and breadcrumb - yes I had not seen / explicit mentioned the other hooks that are not in the constructor - but these are also relevant.

When I test the patch with a scenario that will generate a log message the issue still appears, this time triggered by the call to alter raven_filter (and then raven_breadcrumb)

If the middleware does generate a log message, then the call to ->log is still happening before Drupal has bootstrap so can trigger the same issue. Would need to deprecate and wrap these 2 calls in a similar check to completely cover logging in Middleware.

I did a quick test just commenting out those 2 alter calls and it resolved the issue for the scenario where logs are generated before Drupal has loaded the module files.

Happy to help with next iteration on the patch if you like - would it be adding similar config options for disable_deprecated_breadcrumb_alter and disable_deprecated_filter_alter or just use 1 config flag?

Let's just have one disable_deprecated_alter setting and deprecate the other two as well. Feel free to work on this if you want to, or I'll circle back when I can.

Thanks - I've changed disable_deprecated_options_alter to disable_deprecated_alter and added the check around the $this->moduleHandler->alter('raven_breadcrumb', $breadcrumb); and $this->moduleHandler->alter('raven_filter', $filter);

Leaving as needs work as it still needs updates to raven.api.php and README.md to mark these hooks as deprecated. Focused on additional testing for this first before addressing those 2 points.

Ah and theres a test relying on raven_test_raven_filter_alter

Tweak description of new config

For some reason the README doesn't mention the ignored channels setting - might as well fix that here.

patch didn't fix the issue of http_middleware you could test it by downloading Cloudflare which has

  logger.channel.cloudflare:
    parent: logger.channel_base
    arguments: ['cloudflare']

  http_middleware.cloudflare:
    class: Drupal\cloudflare\CloudFlareMiddleware
    arguments: ['@config.factory', '@cache.data', '@http_client', '@logger.channel.cloudflare']
    tags:
      # Same priority as reverse proxy middleware (http_middleware.reverse_proxy).
      - { name: http_middleware, priority: 300 }

inside his services.yml that calls raven inside logger.factory which causes some of tokens to disappear after clearing the cache for example node:field_image:thumbnail note that if raven is disabled tokens works fine

Automatically closed - issue fixed for 2 weeks with no activity.