-
-
Notifications
You must be signed in to change notification settings - Fork 238
Add Sentry BreadcrumbHandler
support
#489
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
Conversation
7d533ee
to
10c35d5
Compare
ce023ed
to
133cd1c
Compare
133cd1c
to
c469777
Compare
c469777
to
7572a98
Compare
Should we make an exception for |
->booleanNode('persistent')->end() // socket_handler | ||
->scalarNode('dsn')->end() // raven_handler, sentry_handler | ||
->scalarNode('hub_id')->defaultNull()->end() // sentry_handler | ||
->scalarNode('sentry_handler')->defaultNull()->end() // sentry_breadcrumb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a handler
setting used by all handlers that are wrapping another handler to configure which one it is. It should be reused inside of adding a new setting.
I don't think we should add support for third-party Monolog handlers in MonologBundle itself, as this will become unmaintainable:
We already support |
The integration with the 3rd party library was added by #313. Either we decide to deprecate it in 3.x and remove it in 4.x, or we manage to integrate the features. I prefer the first solution for the rasons you exposed, but we need an upgrade path for users. Edit: The documentation of the SentryBundle use the |
I agree that it makes sense not to support 3rd-party handlers. Sentry's is currently the only one, so it's an oddball. However, I don't think 3rd-party handlers are the only issue. For example, this handler seems to be broken: case 'rollbar':
if (!empty($handler['id'])) {
$rollbarId = $handler['id'];
} else {
$config = $handler['config'] ?: [];
$config['access_token'] = $handler['token'];
$rollbar = new Definition('RollbarNotifier', [
$config,
]);
$rollbarId = 'monolog.rollbar.notifier.'.sha1(json_encode($config));
$rollbar->setPublic(false);
$container->setDefinition($rollbarId, $rollbar);
}
$definition->setArguments([
new Reference($rollbarId),
$handler['level'],
$handler['bubble'],
]);
break; The |
@HypeMC this is because this code is related to support of the Monolog RollbarHandler of older versions of Monolog (which we'll be able to clean in 4.0). |
@stof Yes, we should keep support for the handlers themselves. I was thinking we could maybe drop support for registering the 3rd-party classes and just keep the option of providing the service ID (in other words, drop the |
Closing this one as we don't want to support third party handlers. |
This PR was squashed before being merged into the 3.x branch. Discussion ---------- Deprecate `sentry` and `raven` handler types | Q | A | ------------- | --- | Branch? | 3.x | Bug fix? | no | New feature? | no | Deprecations? | yes | Issues | Fix #489 (comment) | License | MIT The upgrade path is to use a "service" handler with `sentry/sentry-symfony` as documented: https://docs.sentry.io/platforms/php/guides/symfony/logs/ Commits ------- 1902f5e Deprecate `sentry` and `raven` handler types
This PR was merged into the 4.x branch. Discussion ---------- Remove `sentry` handler type | Q | A | ------------- | --- | Branch? | 4.x | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #489 (comment) | License | MIT Follow-up #537 Commits ------- 9dd3797 Remove "sentry" handler type
Adds support for getsentry/sentry-php#1199.
Initially, I considered using the existing
handler
option instead of introducing the newsentry_handler
option. However, I decided against it because the PSR logger doesn't get auto-enabled when thehandler
option is used:monolog-bundle/DependencyInjection/MonologExtension.php
Lines 179 to 183 in ed0e4a2