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

[2.0] Get errors only from a specific path & Documentation for construction #458

Closed

Conversation

nokitakaze
Copy link
Contributor

This PR rewrites two old PR. Spice must flow Project must develop. We can not wait for code coverage.

  1. PR Add/Update PHP documentation. #235 by @stefangr
  2. PR get errors only from a specific path #377 by @thamaraiselvam

After merging we will close old ones. I specifically merged here old branches instead of add two different patch.
Personally I don't see PR #377 useful, but @dcramer didn't close the request so we need to merge it

Stefan Grootscholten and others added 7 commits February 8, 2016 12:47
Integrating Raven client in a project when using an IDE is hard because of the lack of documentation.
`$client->setStrictErrorCapturePath($path);`
lower constants
…ature/echos_of_the_past

# Conflicts:
#	lib/Raven/Client.php
#	lib/Raven/Compat.php
#	lib/Raven/Context.php
#	lib/Raven/ErrorHandler.php
#	lib/Raven/Processor.php
#	lib/Raven/SanitizeDataProcessor.php
#	lib/Raven/Serializer.php
#	lib/Raven/Stacktrace.php
#	lib/Raven/Util.php
…os_of_the_past

# Conflicts:
#	lib/Raven/Client.php
@nokitakaze nokitakaze requested a review from ste93cry May 10, 2017 15:35
@ste93cry
Copy link
Collaborator

First of all this mess should be rebased on top of master (if not already done) and then squashed. Said this, what about my comment in #377?

Wouldn't be better to use the send_callback option to customize whether the event originated from a specific source (directory or file) should be sent?

There is alrady an option to prevent exceptions from being logged if their class matches a certain type, and I'm not sure this feature is so useful to justify inclusion in core. We have a callback that can be used to customize what is sent to the Sentry server, if someone wants do to custom things he can use that. I think we have to carefully decide what should go into core, because it makes no sense to accept any feature request without thinking if it can be really useful. Most of the things I saw until now are project-specific enough to not be included here or could be supported by improving the library in other areas instead of focusing on adding that specific feature

@nokitakaze
Copy link
Contributor Author

rebased
squashed

This is not my PRs. They are belong to @stefangr and @thamaraiselvam so I merge their branches and didn't combine them on purpose.

event originated

It could be separate PR. If you want to "map"/"filter" every incoming event we should do filtering function in Client::capture...

We have a callback that can be used to customize

Oh, we already have >_<

Okay. If we don't want to filter Exceptions I could delete this part and then close PR #377

@ste93cry
Copy link
Collaborator

This is not my PRs

I know

I could delete this part and then close PR #377

@dcramer what do you think about this feature?

@thamaraiselvam
Copy link

@ste93cry

There is already an option to prevent exceptions from being logged if their class matches a certain type, and I'm not sure this feature is so useful to justify inclusion in core. We have a callback that can be used to customize what is sent to the Sentry server if someone wants do to custom things he can use that. I think we have to carefully decide what should go into core because it makes no sense to accept any feature request without thinking if it can be really useful. Most of the things I saw until now are project-specific enough to not be included here or could be supported by improving the library in other areas instead of focusing on adding that specific feature

This will be really very useful, I am a plugin developer for WordPress(Around 27% sites in the world running on it) and want to get errors only from my plugin, not from entire WordPress and its impossible to exclude all unwanted paths. Same goes to Magento and Shopify. I hope you got my point.

@nokitakaze
Copy link
Contributor Author

@thamaraiselvam you can do it this way
https://github.com/getsentry/sentry-php/blob/2.0/lib/Raven/Client.php#L1121

But I think we should replace this block to "capture" and rename it as "filter". So we could filter every event we want. I want to move it because of https://github.com/getsentry/sentry-php/blob/2.0/lib/Raven/Client.php#L1029

Filter should be run before storing this event in _pending event. There is no logic to filter events only while we send it. We should do this check before we store/send it.

@Jean85
Copy link
Collaborator

Jean85 commented May 18, 2017

This will be really very useful, I am a plugin developer for WordPress(Around 27% sites in the world running on it) and want to get errors only from my plugin, not from entire WordPress and its impossible to exclude all unwanted paths. Same goes to Magento and Shopify. I hope you got my point.

It's a good idea, but I would generally advise against ignoring all the exceptions coming from outside your code: you're still relying on your framework/CMS, and your code could still do something wrong with external code, and the error could be triggered down the line, outside from your source.

@nokitakaze
Copy link
Contributor Author

against ignoring all the exceptions coming from outside

Closure with filters may check up all stacktrace of the exception and see needed folder in the stacktrace

@nokitakaze
Copy link
Contributor Author

nokitakaze commented May 18, 2017

@thamaraiselvam, @Jean85 because you guys thumbed up my message I draw a conclusion you agree. Okay. I will change this pull request. I will change position of this line https://github.com/getsentry/sentry-php/blob/2.0/lib/Raven/Client.php#L1121 and then I close PR #377

I will add come tests to our tests which will check trivial closure with checking the file of a exception and the entire stacktrace of a exception. So you can see how it works

@thamaraiselvam
Copy link

@Jean85

It's a good idea, but I would generally advise against ignoring all the exceptions coming from outside your code: you're still relying on your framework/CMS, and your code could still do something wrong with the external code, and the error could be triggered down the line, outside from your source.

Yeah, you right but 99.9% error will come from our own plugin, and if we allow other paths then our error reports will be filled up with false data and we cannot check every time whether it comes from our plugin or from other plugins also our plugin is running on 500,000 WordPress sites, If other plugins throws error then our bandwidth (on premises ) and quota of sentry (sentry.io) will be exceeded sooner.

@ste93cry
Copy link
Collaborator

#462, which implements a porting of the Configuration class from the Ruby SDK has a method to check whether an event should be captured or sent to the server (sending_allowed). It should be generic enough to let you implement the callback and prevent events from being sent if they do not match your criteria

@nokitakaze
Copy link
Contributor Author

@ste93cry It is not working at this time, but I will wait for it.

@Jean85 Jean85 added this to the Release 2.0 milestone May 20, 2017
@stayallive stayallive changed the title Get errors only from a specific path & Documentation for construction [2.0] Get errors only from a specific path & Documentation for construction Oct 29, 2017
@Jean85
Copy link
Collaborator

Jean85 commented Oct 30, 2017

#462 is merged, this needs to be either rebased and fixed or closed.

@nokitakaze
Copy link
Contributor Author

I will do it for two days

@nokitakaze
Copy link
Contributor Author

Okay, I just ended to read my PR and all changed from that point...

@nokitakaze nokitakaze closed this Oct 30, 2017
@nokitakaze nokitakaze deleted the feature/echos_of_the_past branch October 30, 2017 19:24
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