Configurable async v2 #116

Merged
merged 2 commits into from Sep 3, 2012

Projects

None yet

2 participants

@cris

Hi @shime,

I've refined configurable async with all your suggestions.

@shime

Why did you choose this instead of

config.async do |job, _notice|
  Thread.new { job.call }
end

Why are you sending job as a parameter that will always get sent, but only sometimes used. What do you think about this instead?

Airbrake.configuration.sender.send_to_airbrake(notice)

It seems a little long, but more self-explanatory to me. We could delegate sender to Configuration in Airbrake to make it shorter.

First version was in a form you suggested, with block. But I thought it is not very intuitive to use default configuration like:

config.async = true

and custom like:

config.async do |...|

with lambda it looks similar, with block it's quite different.
But, if you think it's better to provide block-form, I'm ok and will do appropriate fix.

Again, first edition of what I did was in form:

Airbrake.configuration.sender.send_to_airbrake(notice)

but then I realized you need to know too much details of inner implementation.

Imagine, one day you will change the way Airbrake sends their notice. When we provide job as interface, user doesn't need to do anything and we only need to change the way, how job is created. Of course, for background sender, user should change their implementation, because he should manually pass message into Airbrake delivery routine.

In any way, if you think it's better to avoid job and pass only notice, I'm ok and will do appropriate change.

Hey! Sorry for not responding for some time.

I still think it's cleaner to use block, since we're not using lambdas in configuration block. ignore_by_filters also accepts a block so that makes more sense to me.

We would have to use Airbrake.sender.send_to_airbrake(notice) to refer to the job responsible for sending exceptions inside the async configuration block. I don't know why I added configuration to this call chain. 😄

I think it simplifies the usage of a custom async methods since this block now only accepts one argument, which is self-explanatory.

I don't think this implementation will change much in the future.

Ok, not an issue at all. I'll redo it with all your comments taked into account.

@shime

Again, I think _job is not a necessary argument since it's not used here. And I prefer a block over lambda, since it seems cleaner.

What do you think?

@shime

can you please use if statement instead, so it's more readable?

ok

@shime

nice one!

I think default_async_processor would be a better naming of this method. What do you think about that?

ok

@cris

I've changed branch-name, because, we currently use original 'configurable-async' in production.

@shime shime merged commit 1111f11 into airbrake:master Sep 3, 2012
@shime

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment