Change in vdsm[master]: Adding StatsClient to code base to avoid using python-statsd...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has uploaded a new change for review.

Change subject: Adding StatsClient to code base to avoid using python-statsd package
......................................................................

Adding StatsClient to code base to avoid using python-statsd package

The code is not so complex so I copied it. We use only few parts of it.
I might remove also the rest that we don't use. Quite simple

Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Signed-off-by: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
---
M lib/vdsm/metrics/statsd.py
1 file changed, 52 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/67004/1

diff --git a/lib/vdsm/metrics/statsd.py b/lib/vdsm/metrics/statsd.py
index f471317..24562aa 100644
--- a/lib/vdsm/metrics/statsd.py
+++ b/lib/vdsm/metrics/statsd.py
@@ -19,14 +19,11 @@
 #
 
 from __future__ import absolute_import
+from __future__ import with_statement
 
+import random
 import six
-from vdsm import compat
-
-try:
-    import statsd
-except ImportError as e:
-    raise compat.Unsupported(str(e))
+import socket
 
 _client = None
 
@@ -34,7 +31,7 @@
 def start(address):
     global _client
     if _client is None:
-        _client = statsd.StatsClient(host=address)
+        _client = StatsClient(host=address)
 
 
 def stop():
@@ -45,3 +42,51 @@
 def send(report):
     for name, value in six.iteritems(report):
         _client.gauge(name, value)
+
+
+class StatsClient(object):
+    def __init__(self, host='localhost', port=8125, prefix=None,
+                 maxudpsize=512, ipv6=False):
+        fam = socket.AF_INET6 if ipv6 else socket.AF_INET
+        family, _, _, _, addr = socket.getaddrinfo(
+            host, port, fam, socket.SOCK_DGRAM)[0]
+        self._addr = addr
+        self._sock = socket.socket(family, socket.SOCK_DGRAM)
+        self._prefix = prefix
+        self._maxudpsize = maxudpsize
+
+    def _send(self, data):
+        try:
+            self._sock.sendto(data.encode('ascii'), self._addr)
+        except (socket.error, RuntimeError):
+            pass
+
+    def gauge(self, stat, value, rate=1, delta=False):
+        if value < 0 and not delta:
+            if rate < 1:
+                if random.random() > rate:
+                    return
+            with self.pipeline() as pipe:
+                pipe._send_stat(stat, '0|g', 1)
+                pipe._send_stat(stat, '%s|g' % value, 1)
+        else:
+            prefix = '+' if delta and value >= 0 else ''
+            self._send_stat(stat, '%s%s|g' % (prefix, value), rate)
+
+    def _send_stat(self, stat, value, rate):
+        self._after(self._prepare(stat, value, rate))
+
+    def _prepare(self, stat, value, rate):
+        if rate < 1:
+            if random.random() > rate:
+                return
+            value = '%s|@%s' % (value, rate)
+
+        if self._prefix:
+            stat = '%s.%s' % (self._prefix, stat)
+
+        return '%s:%s' % (stat, value)
+
+    def _after(self, data):
+        if data:
+            self._send(data)


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

Change in vdsm[master]: Adding StatsClient instead of using python-statsd

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding StatsClient instead of using python-statsd
......................................................................


Patch Set 2:

its a simple code and I don't see any benefit in using python-statsd. I prefer to have
my own impl for that. adding uts

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client that sends gauge metrics to statsd port...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client that sends gauge metrics to statsd port using udp
......................................................................


Patch Set 3: Verified+1

it seems to work alright but I can't see the metrics in elk for some reason. checking
that. but the traffic is similar for sure to what we had with python-statsd

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client that sends gauge metrics to statsd port...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client that sends gauge metrics to statsd port using udp
......................................................................


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/3//COMMIT_MSG
Commit Message:

Line 7: Adding simple client that sends gauge metrics to statsd port using udp
Line 8: 
Line 9: The client is separated from base code of the module to allow
Line 10: extensions such as set, counter and rate metrics support. Currently it
Line 11: is just a simple udp client that format simple gauge values.
>  Why do we want to maintain this over using statsd module? It is not
> clear f 
because you don't want to maintain a build for a package that we
don't really use - this code doesn't worth the packaging work
Line 12: 
Line 13: Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client that sends gauge metrics to statsd port...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client that sends gauge metrics to statsd port using udp
......................................................................


Patch Set 4:

better?

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client that sends gauge metrics to statsd port...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client that sends gauge metrics to statsd port using udp
......................................................................


Patch Set 5:

(9 comments)

https://gerrit.ovirt.org/#/c/67004/5/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

Line 57:         self._maxudpsize = maxudpsize
Line 58: 
Line 59:     def _send(self, data):
Line 60:         try:
Line 61:             if data:
>  Why support empty data? is this the original behavior? please
> document this 
why to send anything if the data is empty? I don't understand what
you expect to see here in my reply.. I took this condition from the original code as I
assumed its a right check to do. do you want me to write above it "" if data is
empty we don't do anything "" the first line does that... too many docs is
just confusing
Line 62:                 self._sock.sendto(data.encode('ascii'), self._addr)
Line 63:         except (socket.error, RuntimeError):
Line 64:             pass
Line 65: 


Line 62:                 self._sock.sendto(data.encode('ascii'), self._addr)
Line 63:         except (socket.error, RuntimeError):
Line 64:             pass
Line 65: 
Line 66:     def gauge(self, stat, value):
>  Please document the arguments. 
don't you think its
redundant? ... its quite clear from the context of the code. do you really plan to
document every func you introduce from now on? Im glad to maintain code that is clear by
its own context and not by reading its not updated comments
Line 67:         self._send(self._prepare(stat, '%s|g' % value))
Line 68: 
Line 69:     def _prepare(self, stat, value):


Line 65: 
Line 66:     def gauge(self, stat, value):
Line 67:         self._send(self._prepare(stat, '%s|g' % value))
Line 68: 
Line 69:     def _prepare(self, stat, value):
>  We need this only for testing, and we should avoid this kind of
> testing. 
what? I like this function. prepare creates the string in the format that
fits to statsd. I can add a comment if its not clear enough for you, if that's what
you meant


https://gerrit.ovirt.org/#/c/67004/5/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     def test_unresponsive_destination(self):
Line 28:         # When clients point to not reachable ip nothing should happen.
Line 29:         # we don't want to fail or log in vdsm if server is down
Line 30:         statsd.StatsClient('localhost')._send('a.b.c')
>  Why are using using private methods in the tests? why not use a
> public meth 
because I want to test the internal logic as well... its a unit test,
not functional
Line 31: 
Line 32:     def test_send_gauge_format(self):
Line 33:         def mirror_send(self, data):
Line 34:             if data[-2:] != '|g':


Line 31: 
Line 32:     def test_send_gauge_format(self):
Line 33:         def mirror_send(self, data):
Line 34:             if data[-2:] != '|g':
Line 35:                 raise Exception("gauge format is not correct: %s",
data)
>  This is the entire gauge format? I think we should be more specific.
yes this is all the difference between qauge to sets and counters.. you can read in
the metrics spec
Line 36: 
Line 37:         c = statsd.StatsClient('localhost')
Line 38:         with MonkeyPatchScope([(statsd.StatsClient, '_send',
mirror_send)]):
Line 39:             c.gauge('a.b.c', 1)


Line 34:             if data[-2:] != '|g':
Line 35:                 raise Exception("gauge format is not correct: %s",
data)
Line 36: 
Line 37:         c = statsd.StatsClient('localhost')
Line 38:         with MonkeyPatchScope([(statsd.StatsClient, '_send',
mirror_send)]):
>  We don't need to monkeypatch a class when we have an instance,
> you can simp 
I prefer to monkeypatch.. more clear to me than override the method.
its good for keeping the instance the same if later you add more logic to the test.
about your suggestion - do we want to test socket package?... I guess not. we just want to
see if the send method gets the data as expected. I assume here that socket.sendto func is
tested by python guys
Line 39:             c.gauge('a.b.c', 1)
Line 40: 
Line 41:     def test_prepare(self):
Line 42:         c = statsd.StatsClient('localhost')


Line 39:             c.gauge('a.b.c', 1)
Line 40: 
Line 41:     def test_prepare(self):
Line 42:         c = statsd.StatsClient('localhost')
Line 43:         s = c._prepare('aaa', 1)
>  Don't test private methods, test the behavior of the client using
> the publi 
again... its unit tests. uts should call all internal functions to cover
as much as possible of the code and the possible inputs. probably I don't cover all
here, but those tests are important to keep my planed behavior of those functions
Line 44:         self.assertEqual(s, 'aaa:1')
Line 45: 
Line 46:     def test_module(self):
Line 47:         statsd.start('localhost')


Line 44:         self.assertEqual(s, 'aaa:1')
Line 45: 
Line 46:     def test_module(self):
Line 47:         statsd.start('localhost')
Line 48:         statsd.send({})
>  What does this test? 
exactly what you asked before - testing
the public functions of the module. see that it doesn't raise or fail
Line 49: 
Line 50:     def test_stop(self):
Line 51:         statsd.start('localhost')


Line 48:         statsd.send({})
Line 49: 
Line 50:     def test_stop(self):
Line 51:         statsd.start('localhost')
Line 52:         statsd.stop()
>  What does this test? 
same


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client that sends gauge metrics to statsd port...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client that sends gauge metrics to statsd port using udp
......................................................................


Patch Set 5: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/67004/5/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

PS5, Line 62: 'ascii'
doesn't the statsd server support utf8? this may burn if you want to add Hebrew VM
names.


Line 59:     def _send(self, data):
Line 60:         try:
Line 61:             if data:
Line 62:                 self._sock.sendto(data.encode('ascii'), self._addr)
Line 63:         except (socket.error, RuntimeError):
catch-all except is evil. logging the error is bare minimum - unless you know exactly
which exception you are catching, and why.

In this case, both exceptions seem too broad to me.
Line 64:             pass
Line 65: 
Line 66:     def gauge(self, stat, value):
Line 67:         self._send(self._prepare(stat, '%s|g' % value))


Line 63:         except (socket.error, RuntimeError):
Line 64:             pass
Line 65: 
Line 66:     def gauge(self, stat, value):
Line 67:         self._send(self._prepare(stat, '%s|g' % value))
I'm a bit worried that the user must take care not to include the pipe symbol in they
value. Either add a check, or document the legal values.
Line 68: 
Line 69:     def _prepare(self, stat, value):


https://gerrit.ovirt.org/#/c/67004/5/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 44:         self.assertEqual(s, 'aaa:1')
Line 45: 
Line 46:     def test_module(self):
Line 47:         statsd.start('localhost')
Line 48:         statsd.send({})
>  exactly what you asked before - testing the public functions of the
> module. 
but sending the empty string (and doing nothing) is not the most interesting
thing this module could do. better mock _send and track that several items are really sent
properly.
Line 49: 
Line 50:     def test_stop(self):
Line 51:         statsd.start('localhost')


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 7: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/7/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

Line 59:     def _send(self, data):
Line 60:         try:
Line 61:             self._sock.sendto(data.encode('utf-8'), self._addr)
Line 62:         except (socket.error):
Line 63:             # we allow socket error such as destination not reachable
>  should't we log it at least once? 
no. adding a comment that
explains the motivation
Line 64:             pass
Line 65: 
Line 66:     def gauge(self, stat, value):
Line 67:         '''


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 8: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 8:

(3 comments)

https://gerrit.ovirt.org/#/c/67004/8/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

PS8, Line 76: .decode('utf-8')
why do you need to decode the input? if it is a unicode string as the docstring says, it
needs to be *encoded* in _send.


https://gerrit.ovirt.org/#/c/67004/8/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 34:     def reflect_send_params(self, data):
Line 35:         global global_holder
Line 36:         global_holder = data
Line 37: 
Line 38:     @mock.patch('vdsm.metrics.statsd._StatsClient._send',
reflect_send_params)
in this, you mock your own code, instead of testing it. Instead, you should mock the
_sock.sendto() so no external traffic takes place, but you can still assert which byte
where attempted to be sent. (I know, it is easier said than done)
Line 39:     @permutations((('hello', 3),
Line 40:                   ('hellגד.abc', 9),
Line 41:                   ('גד.abc', 9)))
Line 42:     def test_gauge(self, stat, val):


PS8, Line 42: gauge
the name is not so good, since you do not call gauge() directly. this is a test of the
module-level send() with a single-key dictionary.


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/8/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 34:     def reflect_send_params(self, data):
Line 35:         global global_holder
Line 36:         global_holder = data
Line 37: 
Line 38:     @mock.patch('vdsm.metrics.statsd._StatsClient._send',
reflect_send_params)
>  in this, you mock your own code, instead of testing it. Instead, you
> should 
this needs some more work, but that's what I had in mind:

class StatsdModuleTest(TestCaseBase):

    @classmethod
    def setup_class(cls):
        cls.old_socket = statsd.socket.socket
        cls.mock_socket = statsd.socket.socket = mock.Mock()
        statsd.start('localhost')
        cls.mock_socket.assert_called_once_with(2, 2)

    @classmethod
    def teardown_class(cls):
        statsd.socket.socket = cls.old_socket

    def _helper(self, data, expected):
        self.mock_socket.reset_mock()
        statsd.send(data)
        self.mock_socket.return_value.sendto.assert_called_once_with(
                         expected, ('127.0.0.1', 8125))

    def test_send_single(self):
        stat = 'hello'; val = 3
        data = {stat: val}
        self._helper(data, b'hello:3|g')

    def test_send_hebrew(self):
        stat = 'שלום'; val = 3
        data = {stat: val}
        self._helper(data, b'\xd7\xa9\xd7\x9c\xd7\x95\xd7\x9d:3|g')

    def test_send_multiple(self):
        data = {'hello': 7, 'goodbye': 11}
        self._helper(data, b'hello:3|g')
Line 39:     @permutations((('hello', 3),
Line 40:                   ('hellגד.abc', 9),
Line 41:                   ('גד.abc', 9)))
Line 42:     def test_gauge(self, stat, val):


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 8:

(4 comments)

https://gerrit.ovirt.org/#/c/67004/8/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

Line 48: 
Line 49:     Currently supports only gauge reports which is used in VDSM.
Line 50:     """
Line 51:     def __init__(self, host, port=8125, maxudpsize=512, ipv6=False):
Line 52:         fam = socket.AF_INET6 if ipv6 else socket.AF_INET
>  Perhaps allow also udp over unix domain sockets, might be more
> reliable tha 
requires changes in the listener side collectd-statsd plugin. will
check that in later improvement
Line 53:         family, _, _, _, addr = socket.getaddrinfo(
Line 54:             host, port, fam, socket.SOCK_DGRAM)[0]
Line 55:         self._addr = addr
Line 56:         self._sock = socket.socket(family, socket.SOCK_DGRAM)


Line 62:         except socket.error:
Line 63:             # Keeping python-statsd behavior - we intintially avoid to log
Line 64:             # any socket error to allow running this code without reachable
Line 65:             # server.
Line 66:             pass
>  Perhaps (optionally?) log (to vdsm log or syslog)? Will ease
> debugging. 
the all idea is not to say anything while this fails ..
Line 67: 
Line 68:     def gauge(self, stat, value):
Line 69:         """
Line 70:         Sending gauge report for specific metric in the format stat:value|g


PS8, Line 76: .decode('utf-8')
>  why do you need to decode the input? if it is a unicode string as the
> docst 
fixed.


https://gerrit.ovirt.org/#/c/67004/8/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 34:     def reflect_send_params(self, data):
Line 35:         global global_holder
Line 36:         global_holder = data
Line 37: 
Line 38:     @mock.patch('vdsm.metrics.statsd._StatsClient._send',
reflect_send_params)
>  this needs some more work, but that's what I had in mind:
thanks for being so detailed... the last one is wrong as it runs sendto for each key
in the dict. I'll add other test cases that needed.
Line 39:     @permutations((('hello', 3),
Line 40:                   ('hellגד.abc', 9),
Line 41:                   ('גד.abc', 9)))
Line 42:     def test_gauge(self, stat, val):


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 9:

unrelated failure

10:09:15 FAIL: test_repeating_if_raises (periodicTests.PeriodicOperationTests)
10:09:15 ----------------------------------------------------------------------
10:09:15 Traceback (most recent call last):
10:09:15   File
"/home/jenkins/workspace/vdsm_master_check-patch-fc24-x86_64/vdsm/tests/periodicTests.py",
line 134, in test_repeating_if_raises
10:09:15     self.assertTrue(completed)
10:09:15 AssertionError: False is not true

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 9:

(4 comments)

https://gerrit.ovirt.org/#/c/67004/9/tests/statsd_tests.py
File tests/statsd_tests.py:

PS9, Line 34:        # we call socket.socket(family, socket.SOCK_DGRAM)
            :         self.mock_socket.assert_called_once_with(socket.AF_INET,
            :                                                  socket.SOCK_DGRAM)
not an important part of this module.


Line 39:     @classmethod
Line 40:     def teardown_class(self):
Line 41:         statsd.socket.socket = self.old_socket
Line 42: 
Line 43:     def _helper(self, data, expected):
kill!
Line 44:         self.mock_socket.reset_mock()
Line 45:         statsd.send(data)
Line 46:         self.mock_socket.return_value.sendto.assert_called_once_with(
Line 47:             expected, self._address)


PS9, Line 50: self.mock_socket.reset_mock()
I think this can go into setup(self)


Line 68:             b'hello.\xd7\xa9\xd7\x9c\xd7\x95\xd7\x9d.ma:3|g',
self._address)
Line 69: 
Line 70:     def test_send_long_metric_name(self):
Line 71:         self.mock_socket.reset_mock()
Line 72:         long_metric_name = '1234567890.1234567890.1234567890.1234567890.'
\
( 'xxx'
  'yyy' )

is nicer than a trailing backslash
Line 73:                            '1234567890.1234567890.1234567890.1234567890.'
\
Line 74:                            '1234567890.1234567890.1234567890.1234567890'
Line 75:         data = {long_metric_name: 3}
Line 76:         statsd.send(data)


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 10: Verified+1

failure is not related

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 11: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 11:

(3 comments)

https://gerrit.ovirt.org/#/c/67004/11/lib/vdsm/metrics/statsd.py
File lib/vdsm/metrics/statsd.py:

Line 52:         fam = socket.AF_INET6 if ipv6 else socket.AF_INET
Line 53:         family, _, _, _, addr = socket.getaddrinfo(
Line 54:             host, port, fam, socket.SOCK_DGRAM)[0]
Line 55:         self._addr = addr
Line 56:         self._sock = socket.socket(family, socket.SOCK_DGRAM)
>  You never close this socket. 
adding that
Line 57:         self._maxudpsize = maxudpsize
Line 58: 
Line 59:     def _send(self, data):
Line 60:         try:


Line 62:         except socket.error:
Line 63:             # Keeping python-statsd behavior - we intintially avoid to log
Line 64:             # any socket error to allow running this code without reachable
Line 65:             # server.
Line 66:             pass
>  Are are we going to debug this when you hide the errors like this?
nice logic that im not sure how it will effect. will consider it in later
improvement
Line 67: 
Line 68:     def gauge(self, stat, value):
Line 69:         """
Line 70:         Sending gauge report for specific metric in the format stat:value|g


https://gerrit.ovirt.org/#/c/67004/11/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 25: 
Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     @classmethod
Line 28:     def setup_class(self):
Line 29:         self.old_socket = statsd.socket.socket
>  You are depending on internal implementation detail of the module. If
> we ch 
isn't that good? this is the difference between unit test to functional.
we test the internal implementation here to be as expected
Line 30:         self.mock_socket = statsd.socket.socket = mock.Mock()
Line 31:         statsd.start('localhost')
Line 32:         self._address = ('127.0.0.1', 8125)
Line 33: 


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 14: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 14: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/67004/11/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 25: 
Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     @classmethod
Line 28:     def setup_class(self):
Line 29:         try:
>  You are depending on internal implementation detail of the module. If
> we ch 
sending a specific bytestring through a specific wire IS the API of this
module. That's how it interacts with the statsd server
Line 30:             self.old_socket = statsd.socket.socket
Line 31:             self.mock_socket = statsd.socket.socket = mock.Mock()
Line 32:             statsd.start('localhost')
Line 33:             self._address = ('127.0.0.1', 8125)


Line 32:             statsd.start('localhost')
Line 33:             self._address = ('127.0.0.1', 8125)
Line 34:         finally:
Line 35:             statsd.socket.socket = self.old_socket
Line 36: 
>  This does not work, if setup fails, teardown is not called. 
or,
we can patch statsd.socket.socket at the very last line of setup_class so nothing can
possibly fail it.
Line 37:     @classmethod
Line 38:     def teardown_class(self):
Line 39:         statsd.socket.socket = self.old_socket
Line 40: 


Line 44:     def test_send_single(self):
Line 45:         data = {'hello': 3}
Line 46:         statsd.send(data)
Line 47:         self.mock_socket.return_value.sendto.assert_called_once_with(
Line 48:             b'hello:3|g', self._address)
>  Please avoid such constants, not all developers can read this and
> this even 
BiDi is horrible, but there is nothing wrong in Czech, Hebrew or Chinese
sample text. Lorem ipsum is not the only valid value.
Line 49: 
Line 50:     def test_send_unicode(self):
Line 51:         data = {'\xd7\xa9\xd7\x9c\xd7\x95\xd7\x9d': 3}
Line 52:         statsd.send(data)


https://gerrit.ovirt.org/#/c/67004/14/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     @classmethod
Line 28:     def setup_class(self):
Line 29:         try:
Line 30:             self.old_socket = statsd.socket.socket
this must sit before "try"
Line 31:             self.mock_socket = statsd.socket.socket = mock.Mock()
Line 32:             statsd.start('localhost')
Line 33:             self._address = ('127.0.0.1', 8125)
Line 34:         finally:


Line 31:             self.mock_socket = statsd.socket.socket = mock.Mock()
Line 32:             statsd.start('localhost')
Line 33:             self._address = ('127.0.0.1', 8125)
Line 34:         finally:
Line 35:             statsd.socket.socket = self.old_socket
this must take place only during "except:"
Line 36: 
Line 37:     @classmethod
Line 38:     def teardown_class(self):
Line 39:         statsd.socket.socket = self.old_socket


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 15: Verified+1

(1 comment)

https://gerrit.ovirt.org/#/c/67004/15/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 24: 
Line 25: 
Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     @classmethod
Line 28:     def setup_class(self):
>  Why are you reinventing monkeypatch.MonkeyPatch? 
the test code
looks good enough, no? monkeypatch doesn't fit here to the way I use the Mock(). 

I should do something like
 @MonkeyPatch(statsd.socket, 'sendto', self.mock_socket.sendto)

but then I need mock_socket to be in module level.
Line 29:         self.old_socket = statsd.socket.socket
Line 30:         self.mock_socket = statsd.socket.socket = mock.Mock()
Line 31:         try:
Line 32:             statsd.start('localhost')


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 15:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/15/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 24: 
Line 25: 
Line 26: class StatsdModuleTest(TestCaseBase):
Line 27:     @classmethod
Line 28:     def setup_class(self):
>  Why do you need module level? what is wrong with monkeypatch per
> test? 
I can't use self in the decorator
Line 29:         self.old_socket = statsd.socket.socket
Line 30:         self.mock_socket = statsd.socket.socket = mock.Mock()
Line 31:         try:
Line 32:             statsd.start('localhost')


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 15:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/15/tests/statsd_tests.py
File tests/statsd_tests.py:

Line 38:     def teardown_class(self):
Line 39:         statsd.socket.socket = self.old_socket
Line 40: 
Line 41:     def setUp(self):
Line 42:         self.mock_socket.reset_mock()
>  You can use statsd.socket.reset_mock(), no need for self. 
first
of all we mock statsd.socket.socket , second, i don't understand what you suggest.. to
remove self.mock_socket and above each test to add @MonkeyPatch(statsd.socket.socket,
'sendto', mock.Mock()) ? 

well, if yes, its totally uglier imo and, it doesn't work
Line 43: 
Line 44:     def test_send_single(self):
Line 45:         data = {'hello': 3}
Line 46:         statsd.send(data)


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 15:

(1 comment)

https://gerrit.ovirt.org/#/c/67004/15/tests/statsd_tests.py
File tests/statsd_tests.py:

PS15, Line 28: self
"self" is a confusing same. @classmethod should take "cls" as its
arg.


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 16: Verified+1

(1 comment)

https://gerrit.ovirt.org/#/c/67004/15/tests/statsd_tests.py
File tests/statsd_tests.py:

PS15, Line 28: obj)
>  "self" is a confusing same. @classmethod should take
> "cls" as its arg. 
ok


-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 17: Code-Review+2

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 17: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Yaniv Bronhaim <ybronhei(a)redhat.com&gt;:

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Patch Set 17:

ping? seems ready for merge to me..

-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: Adding simple client for sending gauge metrics to statsd por...

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Adding simple client for sending gauge metrics to statsd port using udp
......................................................................


Adding simple client for sending gauge metrics to statsd port using udp

Introducing simple udp client. The client sends gauge metric statistics in
the metric 2.0 spec format - key:value|g.

Prior to this implementation we used python-statsd package which  is not built
out side epel. The code we used to use from python-statsd is only the part
that sends gauge by udp, we don't have any use for the rest currently.

Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Signed-off-by: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
---
M lib/vdsm/metrics/statsd.py
A tests/statsd_tests.py
2 files changed, 118 insertions(+), 11 deletions(-)

Approvals:
  Yaniv Bronhaim: Verified
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved



-- 
To view, visit https://gerrit.ovirt.org/67004
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I7bbc8a4c0e3afc25c205bfec58c5e77096803404
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Yedidyah Bar David <didi(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;