search

sensu / sensu-go-graphite-handler

MIT License
4 stars 5 forks source link

Entity name with multiple dots (.) - only first one is replaced with '_' #11

Closed anatolijd closed 3 years ago

anatolijd commented 3 years ago

Hi nixwiz, what is the purpose of replacing only first '.' in entity name ? https://github.com/nixwiz/sensu-go-graphite-handler/blob/345aa060102c8a08059fd551b13378f153d4e7a7/main.go#L141

In my environment, entities are FQDNs (is-1234.acme.com) and I'm getting weird Graphite metrics names like

sensu.is-1234_acme.com.vmstat.cpu.waiting 0.000000 1604502632
sensu.annotations.events.is-1234_acme.com.vmstat.action.passing 1 1604502632

I would expect entity name 'is-1234.acme.com' to be converted to single group 'is-1234_acme_com' rather than 'is-1234_acme' and nested 'com' : 

sensu.is-1234_acme_com.vmstat.cpu.waiting 0.000000 1604502632
sensu.annotations.events.is-1234_acme_com.vmstat.action.passing 1 1604502632

Fix would be trivial, I can submit PR but first I want to know why it is so. May be there was some logic behind this ?

nixwiz commented 3 years ago

Actually, I'm not entirely sure any more as it's been a while since I've touched this code. It certainly does not seem right, maybe it should have been -1 instead of 1?

anatolijd commented 3 years ago

That's right. If I were the author, my first intention would be to replace all '.' occurences, and I wonder why it wasn't done initially :) Maybe this code was written and tested with some specific use-case in mind, maybe smth else. In any case, I think replacing all dots with underscore for entity name is correct and expected behavior.

Can you make that change or want me to send trivial PR for -1 ?

nixwiz commented 3 years ago

I'll make that change, and if it breaks things for anyone maybe we'll get an idea as to why it was done this way.

nixwiz commented 3 years ago

@anatolijd version 0.5.1 has been released with this fix.