search

phptek / silverstripe-sentry

Flexible Sentry compatible bug aggregation client for Silverstripe applications.
BSD 3-Clause "New" or "Revised" License
12 stars 22 forks source link

Support minimal log level config property #23

Closed G-Rath closed 4 years ago

G-Rath commented 5 years ago

Typically Sentry clients let you set a minimal log level, which means only events of that level or higher will be reported.

From what I can tell in exploring the code, this isn't support as a configuration option. If it is, it isn't documented.

It'd be great to have this option, as currently we have no control over if things get reported to Sentry (such as PHP warnings).

phptek commented 5 years ago

Thanks for the report. From what I recall of the Raven SDK, this shouldn't be too hard.

What would be the best way of configuring this? YML config or constructor opts?

G-Rath commented 5 years ago

Ideally I'd say have it as a config, b/c its something that you'd change more often based on environments than anything else, but I've not actually used SilverStripe, so I don't know what the impact or use case of a config vs constructor parameter is.

In an ideal world, having both would be great, but I can ask some of the developers at work who just finished working on our first major SilverStripe project if they have a recommendation.

phptek commented 5 years ago

YML config is usually the way to go amd for the reasons you state. Thanks, I'll give it a crack

G-Rath commented 5 years ago

Any progress of this? We've getting a lot of "info" events from Sentry to the point that we're considering removing the plugin.

If I get some time, I'm happy to try and contribute, but can't promise much since I've not worked w/ SilverStripe myself.

phptek commented 5 years ago

I've been concentrating on upgrading to sentry-SDK 2.0 (in branch 3.0). Once that's done (it kind of is, it works) then this is my next priority.

G-Rath commented 5 years ago

Awesome, thanks for the speedy response.

Let me know if there's anything we can do to help (I do know PHP, just not SilverStripe 🙂)

phptek commented 5 years ago

No worries. The last thing I want is for people to abandon or fork the module. That was the reason I created it!

phptek commented 5 years ago

Hi @G-Rath I have pushed some patches to the 3.0 branch, and updated the README accordingly. Apologies for the wait and apologies to your devs that I don't have time atm to test this out. I worked from the official sentry-sdk docs, so should be OK...

Notes:

Cheers for now!

phptek commented 4 years ago

@G-Rath I have tagged 3.0.0-alpha1. Please advise if this is working as expected.

G-Rath commented 4 years ago

@phptek Thanks for that.

Sorry for the long delay and silence - I'll try and give this a test when I have a chance, but unfortunately we can't actually consume non-stable packages in our applications are they're CWP (it makes for an interesting catch-22).

I'll try to get some spare time to test this locally, but can't promise it'll be as thorough as you'd like 😬

phptek commented 4 years ago

Sure you can. If composer dislikes it, just use an alias.

G-Rath commented 4 years ago

Sadly, NZ Govt doesn't support version aliasing yet 😉

phptek commented 4 years ago

Oh I see. You work for a govt client whose IT rules forbid some particular practice. Would it help if I created a release that didn't contain the word "alpha" in it!?

G-Rath commented 4 years ago

You work for a govt client whose IT rules forbid some particular practice

That's the one :)

Would it help if I created a release that didn't contain the word "alpha" in it!

It would have to be a "stable" release 😬

halles commented 4 years ago

Hey @phptek, with @G-Rath we've been working on helping getting v3 out of alpha so we can use it as well as enabling log levels in v2, so we can use it within our requirements.

This PR fixes this particular ticket: https://github.com/phptek/silverstripe-sentry/pull/29/commits

It also adds a build task to test configuration is working.

Cheers!

phptek commented 4 years ago

Nice work! Thank you. I will try and review this as soon as I can. Quick comment is that the BuildTask uses an incorrect namespace - that's all I spotted so far. I just need access to my sentry instance to properly test. Thanks again!

G-Rath commented 4 years ago

Just so you know, our plan is to use 2.0 in all our existing applications, w/ @halles' log_level PR, and use 3.0 in our latest CWP/SilverStripe project that was kicked off a few days ago.

That should hopefully weed out any other possible bugs w/ 3.0 before going live, while letting us suppress events in our other applications straight away.

When we finish development of our latest project, we'll drop you a line telling you how it went, and you can publish 3.0 (if you're happy w/ that)

phptek commented 4 years ago

TBH it already works well. Today was the first time I actually used 3.x with Sentry itself with a 4.4 project. Once all your collective work is merged, I'm happy for that to be made 3.0.0 if you are?

G-Rath commented 4 years ago

I'm happy w/ how things are too - I also can't actually do anything to stop you releasing 3.0.0 either :P

For us internally it makes sense to use 3.0 on this new project, to test the water - so long as we have the log_level feature in all our projects w/ this plugin, there's no need to take extra risk by shotgun updating everything to 3.0, when we've got an app that's not yet public we can stick it in first :)

Cheers for working w/ us on this, it's all looking pretty solid; this also means we're in a good position if we want to expose any other settings in the same manner.

Let me know if there's any work you'd like done but don't have the time for, as I'd be happy to help :)

phptek commented 4 years ago

Awesome! Thanks! Other than what's in the issue tracker, nothing I can think of right now.