Skip to content

Conversation

nomer
Copy link

@nomer nomer commented Oct 15, 2014

Added support for the groupingHash option. The default groupingHash is the request method and URI (just like the default context). This may change the default grouping behavior, as currently the groupingHash is not sent. This option can be set in the same manner that the context is set.

@ConradIrwin
Copy link
Contributor

Hey @nomer, thanks for this!

Unfortunately this isn't quite how grouping hash should be used. By default the Bugsnag server calculates a grouping hash, and so the default should be not to send a groupingHash.

Also we should probably make it so that the groupingHash can be configured per-exception, rather than on the config object. Either by making it settable from the beforeNotify function, or by reading it out of the metadata that's passed to Bugsnag.notify.

Are you able to make these changes? If not, hopefully one of us will get round to it soon.

@nomer
Copy link
Author

nomer commented Oct 15, 2014

Thanks for the feedback @ConradIrwin ! I inherited the code that uses this library, and it's now apparent that my initial changes were highly influenced by our application's usage.

I'd like to make the changes you're suggesting. If we go with the beforeNotify route, how could the user set a custom groupingHash? It appears that the setBeforeNotifyFunction is compatible with only one param (of type Error). As for the metadata route, if we have the caller pass in the groupingHash in the metadata, we would want to unset it after reading and setting it, right? Otherwise bugsnag will create a tab for it in the Dashboard?

Looking forward to your comments.

@ConradIrwin
Copy link
Contributor

Hi @nomer!

What we do in other notifiers (e.g. bugsnag-ruby) is to let the user pass in an object called overrides that we take out any fields we understand, and then use the rest as meta-data. I don't know if that's idiomatic in PHP, or if you can think of a nicer mechanism for passing multiple different optional parameters by name.

The setBeforeNotify function gets a parameter of type Bugsnag_Error, so we can add a setGroupingHash function to Bugsnag_Error and go from there.

@nomer
Copy link
Author

nomer commented Oct 15, 2014

Hey @ConradIrwin! Do you think it would make sense to extend the setBeforeNotifyFunction to accept an optional 2nd parameter, which could be an array that contains the optional parameters? The setBeforeNotifyFunction could then call call_user_func() with this new optional array as the 3rd parameter if needed.

@ConradIrwin
Copy link
Contributor

@nomer I don't think it's necessary to pass options into setBeforeNotify, what did you want to pass?

In modern PHP people who need data in there can just use a closure, otherwise you can still access global variables inside that method ;).

@nomer
Copy link
Author

nomer commented Oct 16, 2014

@ConradIrwin thanks for the suggestion, I was able to pass the data in using a closure :)

I will submit my pull request for adding the setGroupingHash function to the Error class.

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.

3 participants