Apply the same logic to all metric type for Hostname prefixing #83
Conversation
|
hello @banks ! |
|
Do you need any more information ? |
There was a problem hiding this comment.
Hey, overall I think this looks reasonable.
I'm not really responsible for this repo - I only got Commit so I could get some Consul changes through! Sorry you've not had attention here though!
Could you perhaps flesh out in the description some more of the rationale for why this is important - where is this library used that's effecting you etc. It's a perfectly reasonable change but it can help prioritise and incentivise the relevant teams to work on an issue if we know who is feeling an impact especially!
Thanks
| if conf.EnableHostname && conf.EnableHostnameLabel { | ||
| return nil, fmt.Errorf("both EnableHostname and EnableHostnameLabel can't be true at the same time") | ||
| } | ||
|
|
There was a problem hiding this comment.
Does this risk being a breaking change for clients? Is it important the are mutually exclusive? I can't think of a very food reason to use both but If you are migrating between statsd and prometheus for example you might want both being reported in the interim?
There was a problem hiding this comment.
https://github.com/armon/go-metrics/blob/master/statsd.go#L9 (not sure I linked properly)
Statsd and other sink handle labels on their own
In specific statsd case: append them in the name as a suffix, so if both were provided you could have
"localhost-something-localhost-label2
It could break clients using such configs initializers indeed but it will break the client interface anyway
This fix indeed fix the name of all the metrics except gauges if enableHostname is activated
I don't mind removing it tho: it was more of a safeguard against weird behaviors, what do you think ?
There was a problem hiding this comment.
Since the metrics initialize is "mostly" done at start-up time for app, issues will be easier to detect for existing clients that does use both, compared to not seeing the same metrics name after a go get
but again, if you prefer, I really don't feel it's important to keep this if
|
Hello @banks, sorry for HLing again :( , did you had the time to look at this review ? Should I remove the safeguard ? |
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes m.conraux seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
Use EnableHostname boolean in all types of metrics instead of just gauges