980330 - [pulsebuildmonitor] Change wording from "Connecting to" to "Connected to" when connection has been established

Status: RESOLVED FIXED

(Webtools :: Pulse, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

As discussed in issue #410(https://github.com/mozilla/mozmill-ci/issues/410),
we want to change the wording from "Connecting to" to "Connected to" in pulsebuildmonitor

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Here is the patch. I only touched the log message.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not sure why you are still using this bugzilla component. You are aware that mozmill-automation is hosted on github those days.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, a second look showed me that this bug is not about automation but about pulsebuildmonitor. So please make sure to really file the bug into the correct component the next time.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8386785 [details] [diff] [review]
patch_v1.0

Review of attachment 8386785 [details] [diff] [review]:
-----------------------------------------------------------------

So we cannot simply change this logger statement given that it will be actually printed before the connection happens. So what you want to is to add another logger statement after the connection has been established. Something like 'Connected' should be totally enough.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

I added a log after the connection has been established, 
The output looks like:
>./pulse.py config/staging/l10n.json 
>INFO:automation:Connecting to Mozilla Pulse as "[e-mail]|mozmill_l10n|P5139"...
>INFO:automation:Connected

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

Review of attachment 8387465 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

Review of attachment 8387465 [details] [diff] [review]:
-----------------------------------------------------------------

Given that I'm not an official peer of the package, Jonathan may have to finish this off and land it.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8387465 [details] [diff] [review]
patch_v2.0

Review of attachment 8387465 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, this isn't as easy as it looks.  We don't actually perform the connection to pulse until pulse.listen() is called.  But, that call is blocking, and won't return until an exception is thrown (due to CTRL+C, network error, etc).

The connection doesn't occur until this line is called:  http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131

You could add an optional callback argument to listen() that would be called after a successful connection, or you could add logger support to mozillapulse.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, you are right. I feel bad by missing that critical piece. Thanks Jonathan! Also I agree, an onconnection callback sounds like a good idea here, which will help consumers of Mozilla Pulse to handle their own connection code.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

To add an extra callback will require an update in mozillapulse too. 
I would log the message in already existing callback, pulse_message_received, and set a flag so it will log it only once.
http://hg.mozilla.org/automation/pulsebuildmonitor/file/e64ccc6cc647/pulsebuildmonitor/pulsebuildmonitor.py#l131 
Henrik, what do you think? Can I add the log in the existing method, or add a new callback, and modify the mozillapulse too?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cosmin Malutan from comment #10)
> To add an extra callback will require an update in mozillapulse too. 

Have you read the two comments above from Jonathan and myself? You would have seen that no change of mozillapulse is necessary here. What you have to modify is the listen() method of pulsebuildmonitor:

http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l112

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

I have read that and the code also, but listen method is in mozillapulse tool. I cannot just call the listen method with an callback argument, this will overwrite the existing callback set in  __init__ method.
http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l40

I will prepare two patches one for mozillapulse(where listen method is defined) and one in pulsebuildmonitor where the method is called.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ups, and I failed again. Sounds good. I think I will leave anything else on this bug for Jgriffin. :)

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Jonathan, if I will add multiple callbacks here:
http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131
Do I have to acknowledge the message in each callback or is sufficient in one callback?
http://ask.github.io/kombu/reference/kombu.mixins.html#kombu.mixins.ConsumerMixin

Comment 15

[Jonathan Griffin (:jgriffin)]

For the purposes of this back, you don't want to add another callback to http://hg.mozilla.org/automation/mozillapulse/file/f5fb7628a206/mozillapulse/consumers.py#l131; that list of callbacks is called whenever a pulse message is received.

Instead, you want to create a new type of callback, maybe something like:

def listen(self, callback, on_connect_callback=None):

# ... a bunch of existing code ...

# Create a queue and bind to the first key.
queue = self._create_queue(self.applabel, exchange, self.topic[0])
if on_connect_callback:
    on_connect_callback()
consumer = self.connection.Consumer(queue, callbacks=[self.callback])

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 980330[mozillapulse].patch

Thanks Jonathan, this is patch for mozillapulse, I will add one for pulsebuildmonitor too.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 980330[pulsebuildmonitor].patch

This is the patch for pulsebuildmonitor, should be landed if all is good only after mozillapulse.

Comment 18

[Jonathan Griffin (:jgriffin)]

Not sure if you can land these; if not, I'll do it.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Cosmin doesn't have permissions yet. So yes, please land those. Thanks.

Comment 20

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/automation/mozillapulse/rev/c5fbfb5086e7

Comment 21

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/automation/pulsebuildmonitor/rev/af9f89980883

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

For closing this we have to wait for a new version of mozilla pulse and the for a new version of pulsebuildmonitor.
Jhonathan do you know when it will be the next mozillapulse release?

Comment 23

[Jonathan Griffin (:jgriffin)]

https://pypi.python.org/pypi/MozillaPulse/0.91

Comment 24

[Jonathan Griffin (:jgriffin)]

https://pypi.python.org/pypi/pulsebuildmonitor/0.81

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Thanks Jonathan, we will have just to change the pulsebuildmonitor dependency version in mozmill-ci.
The output is:
>(jenkins-env)cosmin@P5139:~/Projects/Mozilla/mozmill-ci$ ./pulse.py config/production/daily.json 
>2014-04-09T09:11:29Z INFO:automation:Connecting to Mozilla Pulse as "[e-mail]|mozmill_daily|P5139"...
>2014-04-09T09:11:29Z INFO:automation:Connected
I close this as fixed.