Add Bunny-based implementation for AMQP #2

Merged
merged 23 commits into from Apr 10, 2012

3 participants

@roidrage
Travis CI member

Pull request opened for discussion, not for merging.

This adds a new consumer and producer, based on the bunny library. We've had some issues with the amqp library losing messages on the way to RabbitMQ without us noticing. This is an attempt to make these errors, should they come up with Bunny too, more expressive and recognizable for us.

@joshk
Travis CI member

Does this just need to be tested before merging?

@roidrage
Travis CI member

Yes, though I need to finish up the consumer. I know it's not really used but for the sake of having it work, I'm going to add some tests and fix it up.

@joshk
Travis CI member
@joshk
Travis CI member

Got everything working nicely on staging, I think we could merge this in, and instead of creating a Bunny consumer just raise an error. What do you think? Seems odd to create a Consumer when we aren't going to use it.

@roidrage
Travis CI member

What this still needs is validation that the metrics are tracked. If not, you could for now just change it to Metriks.meter('travis.amqp.messages.published').mark and add a rescue block that tracks travis.amqp.messages.published.failed.

@roidrage
Travis CI member

Maybe we should include the name of the queue published to in the metric as well, gives us more fine-grained data.

@joshk joshk swapped out AS::Notifications for Metriks
also changed Bunny Consumer to raise errors on initialization as it should not be used as a Consumer.

(I would have removed it but this would have required some big changes to how AMQP is setup with travis-support, raising an error is good for now)
2406343
@joshk
Travis CI member

@mattmatt wdyt?

@roidrage roidrage commented on an outdated diff Apr 9, 2012
lib/travis/support/amqp/bunny/publisher.rb
+ { :key => routing_key, :properties => { :message_id => rand(100000000000).to_s } }
+ end
+
+ def exchange
+ @exchange ||= Amqp.connection.exchange(name, :type => type.to_sym, :durable => true, :auto_delete => false)
+ end
+
+ def deep_merge(hash, other)
+ hash.merge(other, &(merger = proc { |key, v1, v2| Hash === v1 && Hash === v2 ? v1.merge(v2, &merger) : v2 }))
+ end
+
+ def increment_counter(opts = {})
+ meter_name = 'travis.amqp.messages.published'
+ meter_name = "#{meter_name}.failed" if opts[:failed]
+ Metriks.meter("#{meter_name}.#{routing_key}").mark
+ end
@roidrage
Travis CI member
roidrage added a note Apr 9, 2012

Same as in hub, just use a name instead of opts and default it to nil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@svenfuchs
Travis CI member

Can we merge this? Looks like we'll need to opt into using the bunny adapter anyway, right.

@joshk joshk merged commit e43f432 into master Apr 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment