Cue / scales

scales - Metrics for Python
Apache License 2.0
920 stars 73 forks source link

GraphitePeriodicPusher from 1.0.7 fails in Python3 #35

Closed timtebeek closed 9 years ago

timtebeek commented 9 years ago

Using the below code:

from greplin import scales
from greplin.scales import graphite
import time
import logging
import sys

def main():
    # Configure logging
    logging.basicConfig(level=logging.DEBUG, stream=sys.stdout)
    # Gather stats
    stats = scales.collection('/reporter',scales.IntStat('hits'))
    stats.hits += 1
    # Push stats
    pusher = graphite.GraphitePeriodicPusher('192.168.0.10', 2003, 'mypc')
    pusher.allow("*")
    pusher.start()
    # Do something for a while
    for val in range(100):
        time.sleep(1)
        logging.info(val)
        stats.hits = val

Running in python 2.7.6 correctly sends the data to graphite. Running in python 3.4.0 logs the exact same messages, but fails to send: Nothing is received when watching carbon logs on the graphite server.

What can I do to send data to graphite from python3?

hynek commented 9 years ago

I’ve run into this too, the problem is two-fold:

  1. scales checks for iteritems in https://github.com/Cue/scales/blob/master/src/greplin/scales/graphite.py#L104 to decide whether to send something. iteritems is Python 2 only thus it has to be: if hasattr(value, 'iteritems') or (six.PY3 and hasattr(value, 'items')): (although hasattr should be avoided but that’s a different story altogether).
  2. When sending, in https://github.com/Cue/scales/blob/master/src/greplin/scales/util.py#L100 , it will fail because on Python 3 msg is going to be a str, therefore unicode. So there has to be a guard encoding it to bytes.

IOW, nobody has ever used GraphitePusher with Python 3 until now. :disappointed:

timtebeek commented 9 years ago

Thanks for looking into this! Do you recon it would be easy to work your findings into a pull request or (temporary) fork? I'd be willing to help out if needed..

hynek commented 9 years ago

I’ve submitted a PR, let’s see what the maintainers say. I honestly hope a fork won’t be necessary because I don’t have any time to maintain another project. :)

timtebeek commented 9 years ago

Great work, hope to see this merged soon!