graphite-project / carbon

Carbon is one of the components of Graphite, and is responsible for receiving metrics over the network and writing them down to disk using a storage backend.
http://graphite.readthedocs.org/
Apache License 2.0
1.5k stars 490 forks source link

[BUG] Python3 and line protocol cause Data must not be unicode #884

Closed pkruk closed 4 years ago

pkruk commented 4 years ago

Describe the bug Hi I have a problem with official graphite-statsd docker container which use latest version of carbon. When using line protocol, the twisted library have a problem with encoding:

  File "/opt/graphite/lib/python3.7/site-packages/twisted/internet/base.py", line 913, in runUntilCurrent
    call.func(*call.args, **call.kw)
  File "/opt/graphite/lib/carbon/client.py", line 259, in sendQueued
    self.connectedProtocol.sendQueued()
  File "/opt/graphite/lib/carbon/client.py", line 135, in sendQueued
    self.sendDatapointsNow(self.factory.takeSomeFromQueue())
  File "/opt/graphite/lib/carbon/client.py", line 95, in sendDatapointsNow
    self._sendDatapointsNow(datapoints)
  File "/opt/graphite/lib/carbon/client.py", line 487, in _sendDatapointsNow
    self.sendLine("%s %s %d" % (metric, value, datapoint[0]))
  File "/opt/graphite/lib/python3.7/site-packages/twisted/protocols/basic.py", line 476, in sendLine
    return self.transport.writeSequence((line, self.delimiter))
  File "/opt/graphite/lib/python3.7/site-packages/twisted/internet/abstract.py", line 377, in writeSequence
    raise TypeError("Data must not be unicode")
builtins.TypeError: Data must not be unicode

To solve it I add to a code:

class CarbonLineClientProtocol(CarbonClientProtocol, LineOnlyReceiver):                                                                                                                                             

  def _sendDatapointsNow(self, datapoints):                                                                                                                                                                         
    for metric, datapoint in datapoints:                                                                                                                                                                            
      if isinstance(datapoint[1], float):                                                                                                                                                                           
        value = ("%.10f" % datapoint[1]).rstrip('0').rstrip('.')                                                                                                                                                    
      else:                                                                                                                                                                                                         
        value = "%d" % datapoint[1]                                                                                                                                                                                 
      to_send = "%s %s %d" % (metric,value, datapoint[0])                                                                                                                                                           
      self.sendLine(to_send.encode('utf-8'))   <--- My Change

Will you accept a PR with this change ?

pkruk commented 4 years ago

I created a PR for that bug :)

deniszh commented 4 years ago

Not sure if it's good solution and will not have some bad consequences... @piotr1212 ?

pkruk commented 4 years ago

Well when I dig deeper in a problem, there is a problem with a twisted and porting from 2.7 to python 3. (String are not a byte arrays anymore and twisted had a problem with that) like for example here: https://stackoverflow.com/questions/58764784/running-a-twisted-sample-script-on-python-3-7-macos-raises-exception

deniszh commented 4 years ago

TBH I didn't understand what happened lately... Twisted dropped 2.7 compatibility and latest versions are not working anymore? I do not see anything in release notes.

deniszh commented 4 years ago

And IIRC to_send.encode('utf-8') should work both for 2.7 and 3.x Just not sure why it didn't happened before.

piotr1212 commented 4 years ago

This is in the line client which is only used if you explicitly configure DESTINATION_PROTOCOL = line, it is pickle by default. It never occurred to me to test this.

pkruk commented 4 years ago

Yes I set protocol to line, and I'm receiving data from carbon-c-relay, and sending it to another carbon-c-relay. I create with using carbon-relay some kind of proxy between two carbons-c-relay

deniszh commented 4 years ago

Ah, didn't realize that, mea culpa. Thanks for fixing that!

pkruk commented 4 years ago

Thank you for help and code review!

brivadeneira commented 3 years ago

Hello there!

I was around this issue during a couple of days, sendLine() method from LineOnlyReceiver needs that both, line and delimiter are bytes, in my case line.encode('utf-8') didn't work, an extra change was necesarry: elf.delimiter = b'\r\n'

hope it would be usefull for anybody

pkruk commented 3 years ago

Hi!

Well I can check it, and prepare a PR for that

pkruk commented 3 years ago

@brivadeneira which version of the library you're using? In twisted: https://github.com/twisted/twisted/blob/twisted-19.10.0/src/twisted/protocols/basic.py#L435 By default is b'\r\n', may I ask for more details about the problems you met?

brivadeneira commented 3 years ago

@brivadeneira which version of the library you're using? In twisted: https://github.com/twisted/twisted/blob/twisted-19.10.0/src/twisted/protocols/basic.py#L435 By default is b'\r\n', may I ask for more details about the problems you met?

Sorry about the delay, my issue was around twisted, sending msgs with sockets, (twisted version I am using is 20.3.0) ,I will try just deleting delimiter and letting twisted to use the default.

Thanks!

pkruk commented 3 years ago

Hm interesting in 20.3.0 delimiter is the same: https://github.com/twisted/twisted/blob/twisted-20.3.0/src/twisted/protocols/basic.py#L435 But when are no problems at now, that sounds good :)