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

Disable sentry's default integrations when using it #316

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

Conversation

Taluu
Copy link
Contributor

@Taluu Taluu commented Aug 14, 2019

Monolog already does it, as the request integration that comes with the default configuration of sentry.

Disabling it fixes it. Maybe we should add a marker that says we could activate it ? I doubt the interest but I could be wrong.

poke @B-Galati

Copy link
Contributor

@B-Galati B-Galati left a comment

Choose a reason for hiding this comment

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

Great!

@B-Galati
Copy link
Contributor

In fact it would be good to keep Request Integration enabled as it gives a lot of useful information.

@Taluu
Copy link
Contributor Author

Taluu commented Aug 14, 2019

Thought about it, but as there's already the request processor on monolog, didn't think it'd be useful : https://github.com/Seldaek/monolog/blob/master/src/Monolog/Processor/WebProcessor.php

@B-Galati
Copy link
Contributor

That processor is cool but I think it is doing less than Sentry Request Integration.
Also, the goal of the processor is to add information on every single log while Request Integration will add these informations on each captured event which makes more sense for a tool like Sentry where log context is less readable.
Last thing, it's possible that the SentryHandler will not support the "extra" key from log context in future releases. Should be major one but still interesting to know.

@Taluu
Copy link
Contributor Author

Taluu commented Aug 19, 2019

Last thing, it's possible that the SentryHandler will not support the "extra" key from log context in future releases.

Source ?

Anyway, I managed to readd the request integration. poke @B-Galati @lyrixx

@B-Galati
Copy link
Contributor

Last thing, it's possible that the SentryHandler will not support the "extra" key from log context in future releases.

Source ?

getsentry/sentry-python#457 (comment) -> that links to a comment in sentry-php.

@B-Galati
Copy link
Contributor

IMHO it would simpler to manage with a factory class that handles all cases.

Like this one: https://github.com/B-Galati/monolog-sentry-handler/blob/master/doc/guide-symfony.md#step-1-configure-sentry-hub

@B-Galati
Copy link
Contributor

Also, It is needed to set the global Hub by calling Hub::setCurrent():

// A global HubInterface must be set otherwise some feature provided by the SDK does not work as they rely on this global state
Hub::setCurrent(new Hub($client));

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

👍 LGTM.

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Do not merge, this is not ready yet.

@Taluu Taluu force-pushed the fix/sentry-unregister branch from 839f904 to c83a458 Compare August 26, 2019 09:49
@inverse
Copy link
Contributor

inverse commented Sep 2, 2019

@Taluu needs to be rebased from master due to short array clean up 👍

@Taluu Taluu force-pushed the fix/sentry-unregister branch from c83a458 to 03d3f29 Compare September 3, 2019 07:54
@Taluu
Copy link
Contributor Author

Taluu commented Sep 3, 2019

Done. I'll try to find a way to set the global hub state, even though it's repulsive (through a factory or a configurator, probably)

@jderusse
Copy link
Member

Hello what is the status of this PR? why @lyrixx asked for not merging it?

@Taluu
Copy link
Contributor Author

Taluu commented Mar 15, 2021

IIRC, it was because sentry relies on a global state, preventing from really having multiple handlers. I can't remember having made any progress, but I think maybe they had some on their side.

We will need to check with sentry's sdk.

@lyrixx
Copy link
Member

lyrixx commented Mar 15, 2021

Unfortunately, I can not remember. Sorry.
BTW, je sentry handler that is integrated with monolog is really far from perfect.

In my application, I end up with a full rewrite + a basic "service" handler

@B-Galati
Copy link
Contributor

B-Galati commented Mar 15, 2021

And since then they introduced a new major version of the SDK.

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.

5 participants