search

go-graphite / carbon-clickhouse

Graphite metrics receiver with ClickHouse as storage
MIT License
186 stars 47 forks source link

Tag values converted since 0.11.3 (space replaced with +) #124

Closed Hipska closed 1 year ago

Hipska commented 1 year ago

Since 0.11.3 tag values that contain a space are now stored with the space replaced by a + sign.

This conflicts when having tag values that also have a + sign, and this also breaks existing graphite queries where selecting with space was done.

Spaces are allowed: https://graphite.readthedocs.io/en/latest/tags.html#carbon

msaf1980 commented 1 year ago

@Hipska Actual issue or mistake ?

Hipska commented 1 year ago

What do you mean? Since installing this version, the data in the DB now wrongly has + signs instead of spaces. So I would say actual issue indeed!

msaf1980 commented 1 year ago

@Hipska What receiver that reproduce a problem ? In unit tests for tagged uploader (https://github.com/go-graphite/carbon-clickhouse/blob/master/uploader/tagged.go#L182) no problems with spaces, when I check. In plain tcp spaces don't work (space is a separator), other receivers don't tested at now.

Hipska commented 1 year ago

Oh sorry, I'm using the telegraf_http_json receiver.

msaf1980 commented 1 year ago

Yes, bug is reproduce, i'll fix them.

msaf1980 commented 1 year ago

@Hipska Can You test against current master ?

Hipska commented 1 year ago

I guess you mean #125 should have fixed it? Would have been nice to reference this issue on that PR. I also can't see clearly what the exact change to fix it was, as you also did a lot of other changes like replacing stuff to other packages.

msaf1980 commented 1 year ago

@Hipska Yes, this PR. Fixes in https://github.com/go-graphite/carbon-clickhouse/pull/125/files#diff-b415e37221288268df4327bf6a7e5f9fd89153b3c1f913b1483ab0018361c800R58

msaf1980 commented 1 year ago

Also add tests for escape/unescape cycle (for see that produce original string). https://github.com/go-graphite/carbon-clickhouse/pull/125/files#diff-c712f15e9e4d29bfb013ce058f25bba8b43766b29cb5b37e7d013381e209247eR56

Hipska commented 1 year ago

Hi @msaf1980, yes current master looks okay, no more + sign in the tag values..